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

Drop requirement for plugin «name» when «type» attribute is present #2297

Closed
dmitrii-fediuk opened this issue Nov 8, 2015 · 9 comments
Closed

Comments

@dmitrii-fediuk
Copy link

As I see, the sole purpose of the name attribute is to allow the plugin to be redefined by an extension.
I propose to make the name attribute optional (not required) and use the type attribute value as the name when the name attribute is absent.
It would reduce an amount of code, so a developer can write

<type name='Magento\Framework\App\ActionInterface'>
    <plugin type='Df\Framework\App\ActionInterfacePlugin' />
</type>

instead of

<type name='Magento\Framework\App\ActionInterface'>
    <plugin
        name='SomeDummyName'
        type='Df\Framework\App\ActionInterfacePlugin'
    />
</type>
@antonkril
Copy link
Contributor

Plugin name is required for ability to rewrite plugin type.

@dmitrii-fediuk
Copy link
Author

I have said the same in my 1th sentence. What is about my 2nd sentence?

@davidalger davidalger changed the title Propose to drop the requirement for a plugin's «name» attribute when the «type» attribute is present Drop requirement for plugin «name» when «type» attribute is present Nov 9, 2015
@antonkril
Copy link
Contributor

So if plugin declaration is missing attribute "name", the "type" attribute for such plugin can not be overridden in other module, because "type" now becomes identifier.

To allow "type" override, we need "name" as identifier for all plugins.

@dmitrii-fediuk
Copy link
Author

It can.

  1. Initial plugin:
<type name='Magento\Framework\App\ActionInterface'>
    <plugin type='Df\Framework\App\ActionInterfacePlugin' />
</type>
  1. Overrider:
<type name='Magento\Framework\App\ActionInterface'>
    <plugin
        name='Df\Framework\App\ActionInterfacePlugin'
        type='Override\Framework\App\ActionInterfacePlugin'
    />
</type>

By the way (not the puprose of the issue), the overrider looks more clear when specifying Df\Framework\App\ActionInterfacePlugin as the name instead of an abstract SomeDummyName.

@antonkril
Copy link
Contributor

Oh, I see your idea now. It might work, but It mixes values of different nodes, which is confusing for developer. Also It would require specific customized configuration merger. And it's different from what we do in any configuration type.

Alternative approach would be to remove feature of plugin type rewrite (I don't know scenarios for this), but that would be a breaking change.

@dmitrii-fediuk
Copy link
Author

Yes, 99% of plugins declarations are of type 1

<type name='Magento\Framework\App\ActionInterface'>
    <plugin type='Df\Framework\App\ActionInterfacePlugin' />
</type>

so making the name optional can reduce the total amount of the plugin declaration code by about 25%.

@antonkril
Copy link
Contributor

Just to clarify: I think that making name optional would be confusing for developers. It should either stay as is, or name attribute should be removed at all.

@davidalger
Copy link
Member

The GitHub issue tracker is intended for technical issues only. Please refer to the Community Forums or Magento Stack Exchange site for technical questions. In your case, the programming questions forum is likely the most appropriate. Feel free to reopen this issue if you think you have encountered a bug in Magento 2.

@dmitrii-fediuk
Copy link
Author

I propose to do a good desion before the Release, otherwise it would hard to do it in the next 7-8 years because of compatibility importance.

magento-engcom-team pushed a commit that referenced this issue Mar 29, 2018
[TSG] Upporting for 2.3 (pr7) (2.3.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants