IBX-5905: Implemented LoadContent events#250
Conversation
4f3f5da to
f36b357
Compare
alongosz
left a comment
There was a problem hiding this comment.
Looks almost ok, but I have some naming & query building remarks.
Also ideally if we keep only necessary legacy persistence fixtures changes.
We need to either add new integration test or modify existing one which checks properties after creating and/or loading content type to see that it's set.
alongosz
left a comment
There was a problem hiding this comment.
Looks ok. Let's make CI green.
Small remarks:
e1884e3 to
b8ab8df
Compare
|
Just an thought, would be good to test it on large database to see how this change is performing. @mateuszdebinski I think you have access to large datasets. |
|
@lserwatka from what I've verified, there shouldn't be any problems with performance. I checked all SQL queries to which I added a new column and didn't see any changes in performance. I tested on a database that has a size of 43GB before uploading it to the database |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@mateuszdebinski Could you please add description to PR explaining how this change is related to linked issue? |
|
@mateuszdebinski Rebase is needed here |
98d2ff3 to
be0793c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@mateuszdebinski what is the status here? I see CI is failing and it was never sent to QA. At this point it would need to be rebased to 4.6, if still valid. |
|
@alongosz There's one more thing missing, I'll take care of it this week and do a rebase to 4.6 |
|
Hi @mateuszdebinski, any updates on this one? |
alongosz
left a comment
There was a problem hiding this comment.
Seems that you're trying to solve too much at once. The scope here was to add identifier to ContentInfo. Where that new event is used?
There was a problem hiding this comment.
I have a feeling this is here on purpose so you won't forget to implement a pair of events - before event and event.
Any POV @Nattfarinn or @ibexa/php-dev?
There was a problem hiding this comment.
I didn't add the after event for the loadContent function here, but I will add it if necessary :)
There was a problem hiding this comment.
I would say event layer is complete only if it dispatches Before and After events. Before for pipeline interception and input refinement, After as a reaction and postprocessing.
I would add After event. Thanks @alongosz for catching this 👍 .
|
|
@alongosz This PR is part of the solution to the problem and for this, we will need this Event to verify in taxonomy whether the content type points to taxonomy to verify the appropriate permissions. PR for taxonomy will be added soon. First, I wanted to have a completed PR for core (without merge but confirmed that everything is ok) |
alongosz
left a comment
There was a problem hiding this comment.
This PR is part of the solution to the problem and for this, we will need this Event to verify in taxonomy whether the content type points to taxonomy to verify the appropriate permissions. PR for taxonomy will be added soon. First, I wanted to have a completed PR for core (without merge but confirmed that everything is ok)
@mateuszdebinski in that case the essence of the PR changed. I've changed the title accordingly. If you feel that both of these things (events + content type identifier) are separate, then I'd just extract one of those to separate PR and target one PR on top of another. However ATM for me it's not a requirement.
|
added missing Event to maintain consistency, PR should be ready for full check |
e73382d to
0f06579
Compare
|











v4.5PR is currently introducing one of the 3 things needed to solve the problem described in JIRA. Thanks to the content type identifier in ContentInfo, we will be able to decide whether the content type belongs to a taxonomy. If so, it will allow us to verify whether the user has the appropriate permissions. The permissions will be checked by introducing a new Event triggered after the content is loaded.
Work on the fix includes:
Checklist:
$ composer fix-cs).@ibexa/engineering).