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

Method by name already exists in this class #21

Open
norgeindian opened this issue Feb 4, 2025 · 6 comments
Open

Method by name already exists in this class #21

norgeindian opened this issue Feb 4, 2025 · 6 comments

Comments

@norgeindian
Copy link

We have a third party module, which has the following plugin definition:

    <type name="Magento\Elasticsearch\Elasticsearch5\SearchAdapter\Mapper">
        <plugin name="XXX::updateElasticQuery"
                type="XXX\YYY\Plugin\Elasticsearch\SearchAdapter\Mapper"/>
    </type>
    <type name="Magento\Elasticsearch\ElasticAdapter\SearchAdapter\Mapper">
        <plugin name="XXX::UpdateElasticQuery"
                type="XXX\YYY\Plugin\Elasticsearch\SearchAdapter\Mapper"/>
    </type>

As you can see, the plugin name, is the same for both classes.
Now the issue is, that in Magento 2.4.7-p3, these classes extend from each other:

class Magento\Elasticsearch\Elasticsearch5\SearchAdapter\Mapper extends \Magento\Elasticsearch\ElasticAdapter\SearchAdapter\Mapper

As soon as I now run setup:di:compile, I get the following error message:

 A method by name ____plugin_XXX__updateElasticQuery already exists in this class.                                                                     
  Class Magento\Elasticsearch\Elasticsearch5\SearchAdapter\Mapper\Interceptor generation error

When I deactivate this module here and let Magento create the plugins as usual, I don't get any error anymore.
So I think it would be awesome if a way could be found to handle situations like that.
Anyone an idea how to approach that?

@jambardnishvili
Copy link

jambardnishvili commented Feb 6, 2025

I'm investigating this issue atm, it looks like it has the problem when plugin names are the exact same but the first letter is in the different casing, so for example in my testing it correctly run when trying to generate it with names:

XXX::UpdateElasticQuery
XXX::UpdateElasticQuery

But incorrectly when naming is:

XXX::UpdateElasticQuery
XXX::updateElasticQuery

So the issue is somewhere in the casing

@fsw
Copy link
Collaborator

fsw commented Feb 6, 2025

Hi,

What is the default magento behaviour in this case? Are both plugins executed or does the later overrides the first one defined? If it is overridden does it also override it if same name is in different case ("foo" and "Foo")?

Names of variables and getters for plugins are generated from clean_name field generated in this method:

private function getPluginInfo(CompiledPluginList $plugins, $code, $className, &$allPlugins, $next = null)

So probably we need to add duplication check here and make this duplication check case insensitive.

@norgeindian
Copy link
Author

@jambardnishvili , thanks, awesome investigation.
To be honest, I did not even see, that the names differ this way.
I actually thought they were exactly the same.
So thanks a lot for finding that.

@jambardnishvili
Copy link

jambardnishvili commented Feb 6, 2025

Hi,

What is the default magento behaviour in this case? Are both plugins executed or does the later overrides the first one defined? If it is overridden does it also override it if same name is in different case ("foo" and "Foo")?

Names of variables and getters for plugins are generated from clean_name field generated in this method:

private function getPluginInfo(CompiledPluginList $plugins, $code, $className, &$allPlugins, $next = null)

So probably we need to add duplication check here and make this duplication check case insensitive.

Testing with default Magento.

It seems like both Interceptor's are generated in both cases, triggering wise it does seem to have weird side-effects maybe they are intended though 😆 , for example:

Case 1, when names are same but casing differs:

If i have two controllers which extend each other for example Index, ExtendIndex. and we have a plugin added with same name for each of them.

If i hit the Index endpoint, plugin is triggered only once, but if ExtendIndex is hit, then plugin is triggered twice.

Case 2, when names are exactly same with casing as well:

plugin in both Index, ExtendIndex controllers are only triggered once

Testing with the module.

Case 1, When casing is same:
It looks like only one Interceptor is generated and it is for IndexExtend.

Case 2, When casing differs
Then the origin error pops out that it can't generate the interceptor with the same name

@fsw
Copy link
Collaborator

fsw commented Feb 6, 2025

I think plugins should be inherited so magento behavior seems correct (ExtendIndex has two plugins and Index only one) when casing is same I assume one plugin overrides the other and only one is called so magento is case sensitive here.

So guess we just need a fix for Case 2 (When casing differs).

@jambardnishvili
Copy link

Image

This is my Xdebug result when inspecting how plugins are got for IndexExtend controller, so two plugins with different casing, most likely this is the point where we want to decide do we normalize names into lower string or not

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

No branches or pull requests

3 participants