Fix EZP-25416: Content Object Copy: db->commit called after content/publish

[Plopix]

Hi guys,

https://jira.ez.no/browse/EZP-25416

When listening on some events in the new stack from changes done in the Legacy with the approach used in the eZ Recommendation Bundle, we had a bug: Content Object not found.

Indeed when the commit is done after the publish, if we look into the content repository in the listener the content object is not yet there.

It works on the subtree, the commit is done just before like the fix of this PR:
https://github.com/ezsystems/ezpublish-legacy/blob/master/kernel/content/copysubtree.php#L233

Let me know!

++

[yannickroger]

@Plopix You need to create an issue including steps to reproduce if you want you patch to be merged at some point (needed for QA, release notes, etc.). Please add the issue number to the PR and the commit message.

[Plopix]

Ok! @yannickroger done!

[bdunogier]

Not entirely sure what to think here.

The core issue is probably that we are using a different database connection, and the queries made by the new stack are isolated from the current transaction. If something goes wrong during publishing, we end up with incomplete copies. Given that custom code might be executed here, it could be an issue. Otoh, if subtree copy does it this way, why not...

Ping @glye @andrerom.

[glye]

It doesn't say if they are using the content/publish/before trigger, or content/publish/after. But assuming it's "after" - I also think it should be OK to do it the same way as subtree copy.

Another thing - if their workflow fails before this patch, then it might also fail after it: The object they fetch will exist, but it may not yet be in the published state...? Depending on what their code does that might fail too. Something to consider. But if it works, it works.

[gggeek]

Very quick look and seems a bit weird tbh. Will need to look into it more carefully though. The main concern is: what if any legacy events are fired which cancel the copy? Will any stale data be left in ?

[gggeek]

ps: maybe a per-project workaround could be add a commit() inside a legacy event handler, to help with the specific case?

[Plopix]

@bdunogier ok I think that is the issue indeed.

@glye we don't use any eZ4/5 workflows but just that approach:
https://github.com/ezsystems/EzSystemsRecommendationBundle/blob/master/Resources%2Fconfig%2Fservices.yml#L35

@gggeek I agree it's not a easy change. But I don't agree with the argument related to the cancellation of the copy in a workflow. I would not image that the $db-> commit should be here for that.
And more if we look at the publish action itself there is no db->commit after the content/publish that would secure or allow a cancellation of the publish. (but I might be wrong here)

Additionnaly the content/publish should be safe in term of begin/commit right? I thought it was weird to see the db->commit after the content/publish.

Thanks

[gggeek]

I might be wrong (I admit I did not look deeply into the code), but the basic idea is:

• keep the beings and commits balanced
• do not commit halfway through the publication/copy/whatever process

[andrerom]

+0,5 we unfortunately do need to flush changes somehow before triggering search engine directly or indirectly.

[bdunogier]

Hel-lo. This interesting discussion hasn't been finished, as far as I can tell :-)

Given that it has worked that way on subtreecopy, and has not been a problem since then, I'm gonna say +1 on this. We should have enough plusses, but I'm still interested in motivated minuses, if somebody has one.

[Plopix]

Hey I have almost forgotten that we have this custom patch on staging used everyday and soon in production. I know I can not +1 myself but just an update telling you that we are still using this fix on a daily basis. ;-)

[pkamps]

I don't think it's a good idea to wrap 'eZOperationHandler::execute( 'content', 'publish', )' in a transaction.

You can have custom code running because the publish operation is triggering custom workflow actions. Also, it's locking some tables and the likelihood of 'DB transaction errors' due to simultaneous content edits increases.

There are a few more details here:
https://jira.ez.no/browse/EZP-16340

Isn't there another event you can listening for?

[Plopix]

Hey @pkamps, is that a hidden +1 ? ;-)
If I am not wrong you completely agree with the fix. Because right now in the kernel the "execute content publish" is wrapped. The goal of this PR is to remove this wrapping and let the content/publish outside the transaction.

Did I misunderstand your point?

Thank you!

[pkamps]

Oh god, you're right! I love this pull request!

+1 for sure

[andrerom]

What remains here?

If all is clear I'm +1
See below, might not be the right fix as it indeed risks workflow events not being within transaction any longer, or?

[jjCavalleri]

Forgive me for asking this so late in the discussion, but...
What happens if eZOperationHandler::execute( 'content', 'publish', fails or takes too long to execute?
Is there a risk of a copied object being committed as visible because eZContentObjectTreeNode::updateNodeVisibility( $newNode, $newParentNode ); was not executed or has been delayed?

[Plopix]

Hello @jjCavalleri,

It does not failed in normal condition.
But if you are listening a event in the new stack, you may think an object is ready but it is really not because the commit is not done yet. (in the listener)
For instance a ->loadLocation on the publish will fail.

[jjCavalleri]

Hi @Plopix , that you for the feedback. I fully understand and agree with your point of view.
But... what if the code executed in your listener handles content differently in case it's hidden by superior visibility check?
imho the change makes all sense, but it will still introduce a compatibility issue with previous handler. I am sure that we can solve that with a couple of lines in release notes.

[Plopix]

Hi @jjCavalleri,

But... what if the code executed in your listener handles content differently in case it's hidden by superior visibility check?

You bring an interesting new point that is not part of this fix, in fact even with a commit done after the content/publish (like today) the listener (old or new stack) won't necessarily have the good visibility.
It was true before, it is still true.

And I agree if something crashes in the content/publish, we would have a content object with no node in the DB.

Is there a risk of a copied object being committed as visible

Visibility is at the node level then it won't be any new node anyway (if it crashes)

I might be wrong, but at least here is my opinion ;)

Thank you!