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 BlogPost::onBeforePublish() should also trigger parent::onBeforePublish() if exists #657

Open
wants to merge 1 commit into
base: 3
Choose a base branch
from

Conversation

christopherdarling
Copy link
Contributor

e.g. if Page::onBeforePublish() exists.

…ublish() if exists

e.g. if Page::onBeforePublish() exists.
@dhensby
Copy link
Contributor

dhensby commented May 19, 2021

This won't work if the parent class has the method added via an extension.

@christopherdarling
Copy link
Contributor Author

@dhensby Versioned::publishSingle() calls invokeWithExtensions() so if onBeforePublish() is added via a DataExtension this should still be triggered, no?

@dhensby
Copy link
Contributor

dhensby commented May 20, 2021

Would it? It would see that it exists on BlogPost and so will invoke it there and that would be it... right? If you also added an extension to the parent or to BlogPost would that not be ignored?

@christopherdarling
Copy link
Contributor Author

christopherdarling commented May 20, 2021

Not as far as I understand and have tested

Setup as follows:

  1. Page has onBeforePublish() method + DataExtension with onBeforePublish method
  2. SubPage has onBeforePublish() method + DataExtension with onBeforePublish method

results in the following execution order

  1. Page::onBeforePublish()
  2. SubPage::onBeforePublish()
  3. DataExtension::onBeforePublish() (SubPage as $owner)

Prior to this change:

  1. SubPage::onBeforePublish()
  2. DataExtension::onBeforePublish() (SubPage as $owner)

Note: DataExtension::onBeforePublish() only triggers on SubPage even though it's applied to both SubPage and Page. Is that supposed to be the case?

Edit: here's a link through to invokeWithExtensions incase you want to take a look through too - https://github.com/silverstripe/silverstripe-framework/blob/dcdc25500be729f4340d42f8a4fc797461f862f3/src/Core/Extensible.php#L401-L427

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants