silverstripe-blog icon indicating copy to clipboard operation
silverstripe-blog copied to clipboard

Published 20 hours ago • verified publisher icon silverstripe

FIX BlogPost::onBeforePublish() should also trigger parent::onBeforePublish() if exists

Open christopherdarling opened this issue 3 years ago • 4 comments

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

christopherdarling avatar May 19 '21 13:05 christopherdarling

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

dhensby avatar May 19 '21 18:05 dhensby

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

christopherdarling avatar May 19 '21 19:05 christopherdarling

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?

dhensby avatar May 20 '21 09:05 dhensby

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

christopherdarling avatar May 20 '21 09:05 christopherdarling