-
Notifications
You must be signed in to change notification settings - Fork 34
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
AliasType plugins #15
Conversation
We'll want a generic ContentEntityAliasType instead of NodeAliasType |
/** | ||
* Provides an interface for pathauto alias types. | ||
*/ | ||
interface AliasTypeInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this should then extend PluginConfigurableInterface and PluginFormInterface.
* The module handler to invoke the alter hook with. | ||
*/ | ||
public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) { | ||
parent::__construct('Plugin/AliasType', $namespaces, $module_handler, 'Drupal\pathauto\Annotation\AliasType'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should include the module name in the path here, Plugin/pathauto/AliasType probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Forgot we're not in core anymore. :-)
So interesting to note that I think that the actual configuration form should be Pathauto pattern config entities, not forms provided by the AliasType plugin. We want to get far away from having a 'giant screen of pattern inputs' - see https://www.drupal.org/node/273104#comment-2452322 |
I wonder if we could also use derivatives for not only all the entity types, but all the bundles, and then translated language as well |
I don't understand this. I want to clarify that all I want to do here is replace the existing hook by using plugins. If we want to completely plugify the whole module, this issue will never be done. I don't expect a perfect solution, just some that is at least as good as what is there now (which shouldn't be too hard ;)). |
The forms for patterns should not be in these plugins. They should be config entities. |
Hrm, I guess the plugin could have the configuration form for the pattern, but it should be one pattern form in the plugin, and not a lot of patterns, since we'd be using derivatives for those. |
1aae253
to
a2db9a4
Compare
* The module handler to invoke the alter hook with. | ||
*/ | ||
public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) { | ||
parent::__construct('Plugin/pathauto/AliasType', $namespaces, $module_handler, 'Drupal\pathauto\Annotation\AliasType'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments here changed, parent now expects an interface before the annotation class.
Reviewed. Likely next steps:
|
99b6df4
to
9aa2705
Compare
89cee06
to
d173233
Compare
* The token types. | ||
*/ | ||
public function getTokenTypes(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: missing blank line before }
Sure.
Great. Are you ok to merge? This only adds new classes, and shouldn't be swapping out existing implementations (yet). |
Hm. Not sure about merging it like this already, or if we should continue a bit to see if it actually works. But i guess we can change it just as easily in later pull requests as we would here. Ok then ;) |
Added AliasType plugins with first implementation for node.
No description provided.