search

yireo / Yireo_NextGenImages

44 stars 26 forks source link

Plugin default setOrder #21

Closed Amadeco closed 2 years ago

Amadeco commented 2 years ago

Good evening,

Thank you for your extension, it is working great !

To be sure to be executed in the first place for any other plugins which override the getOutput function.

As an example, to work with m2-boilerplate/Module-Lazy-Load, which is working with picture tag, this PR allow to launch first the plugin of your extension and the plugin of the LazyLoad function after (higher number execute first for the after plugin in Magento). However, plugin of m2-boilerplate/Module-Lazy-Load is executed first and it is not working with your extension.

Not sure it is correct to include natively as we can create a custom module for every situation but setting a default sortOrder make sense.

jissereitsma commented 2 years ago

Thanks for this PR. But are you sure that the reordering of this will work with any other extension out there. For instance, if there is a module that is minifying the HTML contents by removing double-quotes (I'm just shouting out an idea here), it would break this NextGenImages module, because first images would need to be replaced and then the minification should take place.

I understand that sometimes - while implementing third party modules - it is needed to reorder plugins. However, I'm absolutely not convinced that it is good to set the ordering to a specific number in the modules themselves.

Did you know that you can just create your own custom dummy module (module.xml, registration.php) and override the di.xml like you mentioned, without requiring a change in the third party module (like NextGenImages)?

jissereitsma commented 2 years ago

Unfortunately I need to close this. As pointed out earlier, I doubt whether this (a sorting order) is something that belongs to any kind of module by default, but it should be set as part of the implementation. Feel free to re-open to re-discuss.