Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EZP-21599: content/publish transaction counter error #767

Merged
merged 1 commit into from Sep 30, 2013

Conversation

5 participants
@bdunogier
Copy link
Member

commented Sep 20, 2013

http://jira.ez.no/browse/EZP-21599

Alternative patch for #766.

Adds an on-interupt key for the trigger definition, that accepts as an argument a method definition. The pre_publish trigger in content/publish defines commitTransaction as the callback method, committing the unfinished transaction.

Same description and reproduction steps as #766.

TODO

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2013

Note: I would consider this 5.2 material, given than we revert the previous patches.

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2013

Much cleaner indeed!

What about passing the returned status to the method ?
I guess you'll implement onInterrupt() in content operation definition class, won't you ?

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2013

Well, why not. What would we use the return status for ?

About implementing onInterrupt, why would we do that ? It can already run any method, with or without custom parameters.

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2013

What would we use the return status for ?

Would help if one would like to react differently if return status is CANCEL or HALT

About implementing onInterrupt, why would we do that ? It can already run any method, with or without custom parameters.

Then we need an update in operation_definition.php for content 😉

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2013

Yes, I've silently pushed that one, kind of didn't add it ;-)

$className,
$body['on-interrupt']['method'],
( isset( $body['on-interrupt']['parameters'] ) ? $body['on-interrupt']['method'] : array() ),
$operationParameters );

This comment has been minimized.

Copy link
@lolautruche

lolautruche Sep 23, 2013

Contributor

CS: Parenthesis must be on next line

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2013

+1
We must revert the hacks introduced to workaround the initial issue.

And I think you should fix the tests as well 😉 (in a separate commit to ease backport).

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2013

New Travis fail concerns stale cache. Can it be related somehow ?

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2013

Uh... unlikely, it's a parenthesis change...

I don't really see how this could be related, even if we ignore the parenthesis change. I'll rebase and we'll see what the new run says.

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2013

(Note: the tests that had failed in the previous run did pass already).

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2013

@bdunogier Can you please revert the workarounds as well ?

@yannickroger

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2013

+1

@andrerom

This comment has been minimized.

Copy link
Member

commented Sep 24, 2013

@bdunogier Should there be a else clause for ´´´if ( isset( $body['on-interrupt'] ) )´´´ to just ´´´return $bodyReturnValue;´´´? Otherwise the code is not returning anymore unless $body['on-interrupt'] is set, right?

Update: I see that this will happen further down, ignore.
+1

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2013

Hmmm, you are perfectly right !

Good catch.

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2013

@andrerom's remark fixed in 41dcfa8.

@patrickallaert

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2013

+1: Sounds good to me, but I wonder what the remaining "Revert workarounds introduced for the issue" entry is in the "todo list".

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2013

@patrickallaert we have fixed the symptoms of this bug in a few places. We just need to revert these fixes (see the last 2 commits)

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2013

Funny finding... https://github.com/ezsystems/ezpublish-legacy/blob/fix-EZP-21599-publish_op_transaction_counter-v2/lib/ezutils/classes/ezmoduleoperationinfo.php#L561

methods in an operation body can also return... I'm not sure we have this case at all, but if it happens, we will get a fatal error as well.

One way would be to move on-interrupt higher in the publish operation definition, outside the body, and handled interruptions in other cases when the body is iterated over. But this is technically incorrect, since the last 5 steps in the operation body don't require this callback...

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2013

After further investigation, this patch does not fix the error for asynchronous publishing, since sending to the publishing queue is done in a method, not in a trigger.

So the patch does need to cover both.

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2013

The implementation should be complete now. Please review again as it touches deep parts of the system...

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2013

What is $isTransactionStarted for?

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2013

$transactionStarted is used to ensure that the transaction is commited by commitTransaction() only if it has been started.

This is to prevent a double commit if the operation is interrupted in the after/publish trigger, since commitTransaction() is called earlier in the operation body.

The TODO entry you're referring to is about the ezjscore patch that prevents the simplify() method from crashing if the object doesn't have a node. This happens when an embed object is added through ezoe, and an approval workflow prevents it from being published immediately. The fix was part of one of the reverted ones (the issue is linked), and I'd like to fix it in a dedicated issue (also linked).

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2013

$transactionStarted is used to ensure that the transaction is commited by commitTransaction() only if it has been started.

OK, I overlooked it was used here, my bad.

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2013

  • removed the reverts from the PR, they'll be in separate commtis for easier management (they won't do any harm, even if not applied)
  • squashed into one commit
Fix EZP-21599: content/publish transaction counter error
The beginTransaction() step in the content/publish operation
was left uncommited if a publish/before trigger interrupted
the workflow.

This adds an 'on-interupt' key for the operation
that accepts as an argument a method definition.

commitTransaction() is set as the onInterrupt callback
for the content/publish operation. If the operation is interrupted,
this method will be called, and the transaction will be completed

bdunogier added a commit that referenced this pull request Sep 30, 2013

Merge pull request #767 from ezsystems/fix-EZP-21599-publish_op_trans…
…action_counter-v2

Fix EZP-21599: content/publish transaction counter error

@bdunogier bdunogier merged commit 2824c61 into master Sep 30, 2013

1 check was pending

default The Travis CI build is in progress
Details

@andrerom andrerom deleted the fix-EZP-21599-publish_op_transaction_counter-v2 branch Sep 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.