Missing Call to `$procede()`

[astorm]

The following class

#File: app/code/Magento/Widget/Model/Resource/Layout/Plugin.php

is configured as a plugin

#File: app/code/Magento/Widget/etc/di.xml
    <plugin name="widget-layout-update-plugin"
            type="Magento\Widget\Model\Resource\Layout\Plugin" sortOrder="10"/>

However, the plugin's aroundGetDbUpdateString method

public function aroundGetDbUpdateString(
    \Magento\Framework\View\Model\Layout\Merge $subject,
    \Closure $proceed,
    $handle
) {
    return $this->update->fetchUpdatesByHandle($handle, $subject->getTheme(), $subject->getScope());
}

Does not have a called to $procede(). This seems like a bug/error/oversight (although, without the context of why this change was made, it's hard to say, which is why this is an issue and not a pull request).

Not calling $procede() here means that attempts to hook this method via another plugin will fail. Also, the presence of a Magento core team written around method without a call to $procede() may encourage bad practices in third party plugins. Finally, since there's no need for a call to $procede(), this should probably be an afterGetDbUpdateString method, and not an around method,

[antonkril]

While this specific plugin could have been avoided (or implemented with after as you described), having around plugins without call to $proceed will be useful in some cases and is valid example of method rewrite (unavailable in M1). Class rewrites and method rewrites are sometimes required when some extension substitutes behavior of other extension (it has to turn off the substituted extension). Method rewrites are a safer way of doing such conflicting changes. Even if there is such around plugin, all plugins with smaller sortOrder number will be called.

[astorm]

Fair enough -- I'm thinking of this more from the point of view of a module developer whose modules enters a system where the sort order on their around plugin happens to be greater than another module plugin's sort order (where that other plugin lacks the call to the the $procede/chain closure)

From the point of view of the person who installs the module it will seem like this new module doesn't work (or worse, breaks things), when in fact it's the other module that's completely replaced some system functionality that's the problem.

Regardless, knowing what is and isn't being taken care of on the system level is important. Thanks for the clarification!

[fooman]

I think if Magento Core has to achieve such a conflicting change I would at least suggest starting with a higher sortOrder than 10. This could then become part of a best practice guide to extension developers:
Magento Core uses sortOrders above 1000
Published extensions on Connect would use sortOrders in the 100-999 range
and any system specific extensions could use 1-99

[kandy]

@astorm We recommend do not use plugin at all, if you can. Better way to avoid module conflicts its use Interface segregation principle and use integration/functional test that verify that all work a expected.
@fooman Maybe better to use double for sort order. I believe that this allow resolve most of problem with ordering.

[piotrekkaminski]

@alankent can you chime in here? should adopt a policy to have specific sort order range?

[antonkril]

In general plugins should not know about each other or order of execution, so ext developer should not set their sort order explicitly. But that is mostly true only for non-conflicting plugins (before-, after- or around- with unconditional $proceed() call).

Usually conflicting plugins are:

• conditional, that call to $proceed only when some condition is true (action authorization plugin proceeds only if user is allowed to see that page). Such plugins usually want to be closer to beginning of execution chain.
• unconditional, that want to completely replace functionality of pluginized methods (alternative implementation of filesystem read operation). These are method rewrites. And they usually want to be right before the original method is called.

Based on this we can not use 1-99 for system specific extensions, because Magento core uses a lot of conditional plugins that should be executed before system specific extensions or Connect extension plugins (100-999). And Connect extensions and system specific extensions will use unconditional conflicting plugins that should be executed after Magento Core plugins.

One possible rule that I can see is that all unconditional conflicting plugins should all have same big number (1000 ?) for sort order, because if there are 2 unconditional conflicting plugins SI will have to make decision which one should be used. Using module sort order to define which plugin is used for two plugins with same sort order does not seem correct.

[fooman]

Thanks @antonkril for chiming in.

You are mentioning two types of changes that are being done through the plugin system - conflicting and non-conflicting. However the plugin system itself does not have a mechanism to carry this information so I don't think it should necessarily be part of this discussion (maybe something on the wishlist for plugin system 2.0).

This statement here

Based on this we can not use 1-99 for system specific extensions, because Magento core uses a lot of conditional plugins that should be executed before system specific extensions or Connect extension plugins (100-999).

I don't understand. Currently most extensions that you are referring to are not even written yet. What I think you are saying is that if Magento Core creates a conflicting plugin it means it doesn't want to allow for any one else to change the behaviour any further. I think this is doing a disservice to extension developers and SIs as it prematurely limits their options. However if this is the case the implementation should be changed to a DI preference which more clearly indicates that this is a conflicting change (which I think might then open up the possibility for a plugin again).

What my proposal was aimed to be the easiest way to work with the existing system and outline what everyone's playground is. This is an analogy to the dis-established code pools. In an ideal scenario in the past all Magento core changes were under core, Connect extensions under community and site specific code under local. This would map to core -> 1000+, community -> 100-999, local -> 1-99. Giving each group that comes in later a defined area to work in. You probably want to squeeze in Enterprise there somewhere too (for example CE 2000+, EE1000+).

[antonkril]

Magento Core does not use conflicting unconditional plugins (at least does not actively use them). They are for Extension developers. But Magento Core uses conditional plugins that stop execution flow only if some condition is not met. They do not substitute implementation and should not be considered problematic for extension developers. But they usually want to be executed first, so will have low sort order (<1 in your example)

And unconditional plugins substitute behavior. They usually should be executed right before original method (hight sort order, > 1000 to substitute core plugins in your example). They will be used mostly by extension developers and SIs.

Of course interface preference substitution is more explicit way of doing behavior substitution but we are talking here about "method rewrite" feature, when you only need to substitute one method of interface. It is less conflicting than interface preference substitution.

[alankent]

If I try to explain in different words, the scores are a way of controlling relative priority. There was a separate discussion somewhere on the best way to do this (might have been an in person discussion in a partner meeting - I think in Austin). Scores we have today, but maybe it should really be "I want to go before module X (if present)". That is what is really trying to be achieved.

Sometimes an extension will need to go before one of our plugins, sometimes afterwards. That is why just doing a rule like "3rd party gets 1000 onwards" will not work. Sometimes they need to be before us.

I think the real pain is using numbers does not capture semantic intent. Semantic intent is "I want to be before X an and after Y" (constraints would only apply if X or Y were loaded). We had talked about implementing this, but it was too much for GA - scores are the best we have for now. So for now, scores are really "OK, I want to be before X and after Y... they used scores 300 and 100, so I will pick..... 197!"

I recall a part of that original discussion was "as long as system integrators can override the scores, they can fix things as a last resort if there are dependencies the extension developers were not aware of". Hence at the time people felt it was OK to stick with scores. Extension developers may learn over time to get the relative numbers correct, and system integrators can write a module to override them for a particular site if they are wrong.

[astorm]

I haven't seen any action on this in nine months, so am closing it out. Based on what I've seen in the core code it looks like skipping calls to $proceed() is standard, if only occasionally, practice in the core, and that each module developer will need to decide if and when they want to break the plugin calling chain for a feature. @alankent @antonkril -- if you disagree with the above and have an official best practices please re-open and/or use other channels to let the community know.

[magento-engcom-team]

Hi @astorm. Thank you for your report.
The issue has been fixed in #29633 by @Nazar65 in 2.4-develop branch
Related commit(s):

• 9fd19aa5e6210e23d84d36b15f9807a2442a0854
• 084f689c14cc47fae2b53c8f11f87de548e76227
• 363a100196ae516e616d74050945b07f9343552d
• 702d201af93f3560dcfa5d46a209aa2a1d8a1dd8
• 58b7d3b75cc01b51901fbc685db56893ad7aaa08
• 37570780174507183909ff1e8b822b0737f944fc
• a246993700ecca5abbe32128fc2735a5110abdd5
• 773c20c8506c90127eb6c39d642547acfa4e2b48
• 523a8c9865227cfc9d9bdf9a150bb48586704f39
• c7d50bb2f126e8e1fd72541d597eeda0e908aa1d
• 18e7988989037dabb97559d95892cb5f8d555f57
• 8c0e9919f8acaf741a44a0fe7a74cb42276c8d0d
• 9a2880dd11eb8cd6bc4b1afab08d7f4e31bedb96
• 141434f64214fb4bda3262a03fe4e8ed91fcac71
• 22e613ce6486a3e4c291b756cd9c02e04a57aed9
• 338f5105f4158b30e1678e8227b88c61bd0e89e6
• a572253280b0cfe296be7622288cc06453bb171f
• 51188d9a565bada56ed38dd5740f19b387db3029

The fix will be available with the upcoming 2.4.2 release.