In the back of my mind ... there are always been a problem with my IDE's autocomplete when mucking around with create() methods ...
For view plugins the root interface create() annotation is corrupt.
Just for comedy value I am including a list of classes that extends ViewPluginInterface::create()
Drupal\views\Plugin\views\field\Field, Drupal\views\Plugin\views\PluginBase, Drupal\rest\Plugin\views\display\RestExport, Drupal\views\Plugin\views\row\EntityRow, system\Plugin\views\field\BulkForm, Drupal\datetime\Plugin\views\filter\Date, Drupal\views\Plugin\views\display\Page, Drupal\views\Plugin\views\argument\Date, Drupal\user\Plugin\views\access\Permission, Drupal\views\Plugin\views\cache\Time, Drupal\views\Plugin\views\display\PathPluginBase, Drupal\views\Plugin\views\display\Block, Drupal\taxonomy\Plugin\views\argument_default\Tid, Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid, Drupal\views\Plugin\views\field\EntityOperations, Drupal\views\Plugin\views\argument_default\Raw, Drupal\views\Plugin\views\filter\Bundle, Drupal\user\Plugin\views\access\Role, Drupal\user\Plugin\views\field\Permissions, Drupal\block_content\Plugin\views\area\ListingEmpty, comment\Plugin\views\field\NodeNewComments, Drupal\user\Plugin\views\filter\Permissions, Drupal\views\Plugin\views\argument_validator\Entity, Drupal\views\Plugin\views\field\Date, file\Plugin\views\argument\Fid, node\Plugin\views\argument\Vid, Drupal\views\Plugin\views\area\Entity, Drupal\views\Plugin\views\field\LinkBase, Drupal\views\Plugin\views\area\View, Drupal\views\Plugin\views\field\EntityLabel, node\Plugin\views\argument_default\Node, Drupal\rest\Plugin\views\style\Serializer, Drupal\user\Plugin\views\argument_default\User, Drupal\taxonomy\Plugin\views\field\TaxonomyIndexTid, Drupal\taxonomy\Plugin\views\relationship\NodeTermData, Drupal\user\Plugin\views\field\Roles, Drupal\views\Plugin\views\filter\LanguageFilter, comment\Plugin\views\argument\UserUid, node\Plugin\views\area\ListingEmpty, Drupal\user\Plugin\views\argument\RolesRid, Drupal\user\Plugin\views\filter\Roles, Drupal\views\Plugin\views\field\TimeInterval, aggregator\Plugin\views\argument\Fid, aggregator\Plugin\views\argument\Iid, Drupal\taxonomy\Plugin\views\argument\VocabularyVid, Drupal\user\Plugin\views\argument\Uid, node\Plugin\views\argument\Type, node\Plugin\views\argument\Nid, Drupal\views\Plugin\views\row\RssPluginBase, Drupal\user\Plugin\views\field\UserData, Drupal\taxonomy\Plugin\views\argument\IndexTidDepth, Drupal\taxonomy\Plugin\views\argument\Taxonomy, and Drupal\views\Plugin\views\relationship\EntityReverse.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | ViewPluginInterface.png | 23.83 KB | martin107 |
| #2 | create-2634022-2.patch | 1008 bytes | martin107 |
| create-0.patch | 1002 bytes | martin107 |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
martin107 commentedadded the param type mixed.
Comment #3
martin107 commentedComment #4
dawehnerIMHO we should remove that method from the interface, it simply doesn't belong there.
Comment #5
damiankloip commentedAgree, not sure why it is on there now. It shouldn't be. static::create() is the worst kind of injection.
Comment #6
dawehnerWell, IMHO its okayish, its not worse than annotation based injection, which for some reason some people like, but we don't have it in Drupal.
Comment #7
damiankloip commentedYeah, I mean in Drupal at least. I hope we never have annotation based injection...
Comment #8
martin107 commentedI also find create() based injection undesirable,
Taking Drupal\views\Plugin\views\field\LinkBase as an example picked at random
The overridden create() injects the accessManager
There is also an implied injection using \Drupal::currentUser()
I prefer create() to \Drupal::xyz()
It is difficult to undo that design decision at this late stage as it represents a mountain of work ... which grows daily as contrib is railroaded into the bad practice.
Speaking constructively, what we need to do is lobby for the pattern to be removed/deprecated in Drupal9 or later.
It would be better to mark it deprecated sooner rather than later.
Anyway for now, I would still recommend this patch, as it is better to reflect reality.
Comment #12
martin107 commentedI find myself still wanting to resolve this 2 years later
Maybe I should have marked this "needs review" earlier :(
So from #4
And 2 years ago in #8 when D9 was in the far far future ... I want to skip that step and just let the documentation reflect reality -- and document accurately something no one was happy with.
I am including doxygen inheritance diagram so you can see at a glance the complex dependancy we have here...
I am animated enough to want to fix it properly today ....but the diagram shows it would be a big job
@dawehner, @damiankloip
Can I ask what you 2017 opinion is?
Is is reasonable to fix it properly now?
Comment #13
martin107 commented.
Comment #25
dpiRan into this with PHPStan + phpstan/phpstan-strict-rules
The current docblock of {inheritdoc} + create being at the root really confuses PHPStan badly, adding three identical errors for each views plugin with a overriding create method:
Either we should remove
\Drupal\views\Plugin\views\ViewsPluginInterface::create, or make\Drupal\views\Plugin\views\ViewsPluginInterfaceimplement\Drupal\Core\Plugin\ContainerFactoryPluginInterface.I don't think we are permitted to assume projects are always using
\Drupal\views\Plugin\views\PluginBase, which already extendsContainerFactoryPluginInterface, and the method already exists. So leaning towards makingViewsPluginInterfaceextendContainerFactoryPluginInterface.Comment #26
dpiI'll probably add a bunch of things under this tag related to https://github.com/phpstan/phpstan-strict-rules
We already have
PHPStan-0+PHPStan-1Comment #28
dpiPushed MR.
Making this more about PHPStan since its becoming more of a thing.
Comment #29
dpiComment #30
smustgrave commentedSeems like a good change.
Only question is if this will need a change record.
Comment #32
catchLooks good. No need for a change record IMO, the method is already there.