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

RFC: Sylius plugin naming convention - allow Sylius in plugin name #8848

Closed
stefandoorn opened this issue Oct 16, 2017 · 7 comments
Closed
Labels
Feature New feature proposals. RFC Discussions about potential changes or new features.

Comments

@stefandoorn
Copy link
Contributor

stefandoorn commented Oct 16, 2017

Q A
RFC? yes
Sylius version ALL

Currently, the Sylius plugin manual dictates NOT using Sylius in the name: http://docs.sylius.org/en/latest/plugins/creating-plugin.html (at the bottom). It doesn't clearly state how to name the repository of a plugin, but most plugins until now also exclude Sylius from the repository name. The composer.json states type sylius-plugin, but that's only discoverable through Packagist search: https://packagist.org/search/?type=sylius-plugin.

On one of my plugins I got an issue filed to rename the plugin to something more recognisable, basically adding Sylius to the naming - which I can only agree with: stefandoorn/sylius-google-tag-manager-enhanced-ecommerce-plugin#13.

I see multiple benefits from renaming plugins to include the Sylius name:

  • Direct distinction for which platform this plugin is suitable.
  • Easier to spot plugins for potential new users (Google search for plugins will improve e.g.). Plugin adoption can rise, ecosystem improves.
  • More marketing for name Sylius. Seeing the name more around make people wonder what it is and does.
  • Via @pamil (see comment below): The additional benefit would be to have sylius_ prefix in the configuration files. SitemapPlugin only would result in sitemap key, whereas SyliusSitemapPlugin would result in sylius_sitemap which looks less confusing for me (and reduces potential conflicts).

Proposed changes:

  • Repository name should start with sylius- and end with plugin. E.g.: sylius-sitemap-plugin.
  • Namespace can stay as is for me, e.g.: SitemapPlugin, because as soon as a Sylius user is using it it's rather clear.

Alternative for namespacing change:

  • Alternative for namespacing might be: SyliusSitemapPlugin to make it even more clear inside the code that you are using a Sylius specific thing - instead of something Symfony wide of even PHP wide.
  • Another alternative is to require to add the author also to the namespace, to make even more clear it's not maintained by Sylius core devs when browsing through code, e.g.: StefanDoorn\SyliusSitemapPlugin.
@gabiudrescu
Copy link
Contributor

I "vote" yes for the repository naming rules. you had me at Google SEO.

for Namespacing, I would be interested to learn how is this impacting current users of the plugin - isn't this a BC break?

@pamil
Copy link
Contributor

pamil commented Oct 16, 2017

The additional benefit would be to have sylius_ prefix in the configuration files. SitemapPlugin only would result in sitemap key, whereas SyliusSitemapPlugin would result in sylius_sitemap which looks less confusing for me (and reduces potential conflicts).

As for namespaces, I think we should follow PSR-0 / PSR-4 and always include vendor name in the first one so StefanDoorn\SyliusSitemapPluginit is.

I'm not sure how it works from trademark-related perspective so I'd leave it for @pjedrzejewski - it wouldn't be a bad idea to create some sort of explicit trademark policy (like the Symfony one).

@pamil
Copy link
Contributor

pamil commented Oct 16, 2017

@gabiudrescu changing the namespace is a BC break indeed (without providing any deprecation layer first), but plugins do not need to follow Sylius' release cycle, so it's perfectly fine to release new major version then.

@stefandoorn
Copy link
Contributor Author

@gabiudrescu @pamil Yes, this might lead to quite some BC breaks if we start renaming namespaces etc.

@pamil Good one with config naming! I'll edit the RFC to include this one.

@CoderMaggie CoderMaggie added Plugin RFC Discussions about potential changes or new features. labels Oct 16, 2017
@pamil pamil added this to the 1.0 milestone Oct 16, 2017
@pjedrzejewski
Copy link
Member

Everything in this thread makes sense to me. :) @stefandoorn Could you open a PR to the documentation?

@stefandoorn
Copy link
Contributor Author

Sure @pjedrzejewski, will do that later this week.

@stefandoorn
Copy link
Contributor Author

Submitted a PR in #8903. I hope I took all feedback into consideration. Only thing I've adjusted is the repository name, as it now simply reflects the class name / namespace name instead of another format. I think this is easier. Feedback is welcome.

Next to this, a PR should be opened against the PluginSkeleton to rename things in there also probably I guess - but not 100% sure about that.

@pamil pamil added Feature New feature proposals. and removed Feature New feature proposals. Plugin labels Oct 27, 2017
stefandoorn added a commit to stefandoorn/Sylius that referenced this issue Nov 7, 2017
stefandoorn added a commit to stefandoorn/Sylius that referenced this issue Nov 7, 2017
stefandoorn added a commit to stefandoorn/Sylius that referenced this issue Nov 8, 2017
pjedrzejewski added a commit that referenced this issue Nov 9, 2017
Plugin naming convention RFC implementation (closes #8848)
pamil added a commit to pamil/Sylius that referenced this issue Nov 9, 2017
* 1.0:
  Plugin naming convention RFC implementation (closes Sylius#8848)
@pamil pamil closed this as completed in d9e315d Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals. RFC Discussions about potential changes or new features.
Projects
None yet
Development

No branches or pull requests

5 participants