Updated: Comment #N
Problem/Motivation
We added ways store the provider for all views handlers and plugins. This is good for the likes of #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall, as we can use the providers to help build dependencies. However, there is a missing piece; Handlers such as the "field" field handler (confusing, yes) provided by the field module (Drupal\field\Plugin\views\field\Field) will have a provider of 'field' as the field module has provided the plugin. The selected field could then use something like image, for example. So there is actually a dependency on field and image modules. Currently, we have no way of determining this.
Proposed resolution
1. Rely on having the plugin instances available (#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall will just use config data atm - which is probably the right thing to be doing), we could then have a getDependencies() method that is called that can add optional deps.
2. Add a new mechanism when plugins/handlers are added to a view that call something similar to above to get these optional additoinal dependencies, and add that to the configuration for that plugin/handler. Similar to when we add the provider key for example.
Remaining tasks
Everything
User interface changes
None
API changes
Could break some older default views, needing an update to these additional dependencies...
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-2216071.txt | 533 bytes | dawehner |
#31 | 2216071-31.patch | 17.33 KB | dawehner |
#29 | interdiff.txt | 6.98 KB | dawehner |
#29 | vdc-2216071-29.patch | 17.33 KB | dawehner |
#25 | drupal.plugin_deps.2216071.25.interdiff.txt | 4.24 KB | Gaelan |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedSomething like this to start, as a general idea. This is by no means tested or anything.
Comment #2
damiankloip CreditAttribution: damiankloip commentedThis would actually work though. So we bypass defineOoptions/unpackOptions() logic.
Comment #3
dawehnerSHouldn't we add the module providing the handler add automatically to its dependencies?
Additional we should also provide some method which collects all the dependencies of all handlers/plugins in a view. Do you want to work on that in a follow up?
Comment #4
damiankloip CreditAttribution: damiankloip commentedWe can work on that here too, that was literally a 30 second patch :)
We can add the module too, that would keep it in one place. We might want to just rename s/additional_dependencies/dependencies in that case though.
Comment #5
dawehner+1 for the renaming.
Comment #6
dawehnerContinued a bit.
Comment #7
damiankloip CreditAttribution: damiankloip commentedCreated #2219661: Rename ViewExecutable::viewsHandlerTypes to getHandlerTypes. We should do that.
Comment #8
damiankloip CreditAttribution: damiankloip commentedAlso, just created #2219689: Separate out logic for getPluginTypes/viewsHandlerTypes from ViewExecutable in an attempt to help improve the messy situation we have with iterating over different types of views plugins.
Comment #9
catchComment #10
alexpottI think we should be adding config dependency on the field instance or maybe entity display rather than the module. Since the view is broken when the field instance is deleted - not when the module providing the field type is uninstalled.
Comment #11
alexpottwe already have View::calculateDependencies() I think this should be rolled into that.
Comment #12
dawehnerContinued with some work on here
Comment #14
dawehnerLet's fix stuff.
Comment #16
damiankloip CreditAttribution: damiankloip commentedComment #18
damiankloip CreditAttribution: damiankloip commentedLet's try and get a passing patch, and go from there.
Comment #19
alexpottShould be
return array('entity' => array($field->getConfigDependencyName()));
Comment #20
damiankloip CreditAttribution: damiankloip commentedNice, that makes a lot of sense. Will go into the next patch!
Comment #21
dawehnerDo we have a clue why we don't call that at the moment?
Comment #22
tim.plunkettIn this class,
$this->field_info = FieldHelper::fieldInfo()->getField($this->definition['entity_type'], $this->definition['field_name']);
is called in init(). This is called after that, so we can reuse that, right?These seem like they could have used data providers...
Booooo hisssssss
Yay not mucking up defineOptions more.
Comment #23
dawehner@damiankloip
Do you want to bring that one home or should someone else?
Comment #24
Gaelan CreditAttribution: Gaelan commentedI'll give this a shot.
Comment #25
Gaelan CreditAttribution: Gaelan commentedComment #26
Gaelan CreditAttribution: Gaelan commentedComment #29
dawehnerThere we go.
Comment #30
tim.plunkettSo we keep the public property, but leave the method protected?
Assuming we'll still need to access the field definition of a field handler, I'd much rather make the method public and add an @todo to make the property protected.
Otherwise, the patch looks great.
Comment #31
dawehnerSure, let's do it.
Comment #32
tim.plunkettOh wow, I didn't think that would work!
Comment #33
alexpottIf I add the body field to the content view rhe dependencies become
they were:
Adding the field module as well as the field entity is a little odd. Not sure how to fix this though. Setting to needs review to get opinion on whether this is important.
Comment #34
dawehnerIn an ideal world field module would not be required. If we assume this, the user should see that field module is needed, not just a config entity with a quite obscure name.
Do you suggest to get rid of required modules as dependencies?
Comment #35
xjmI don't think it's a bad thing to declare a dependency on the field module. It's for the present a required module, but so is user, and we're declaring a dependency on that. Having the explicit dependency seems correct to me, since the field module is providing the needed plugins as well as the entity the view depends on. Sure, the entity it depends on already has its own dependency on the module, but I don't see anything wrong with it. The important thing is that field.module, its plugins, and its relevant namespaced config entities be available when we work with the view.
NB: I personally haven't reviewed the patch in depth, just @dawehner and I agreeing on a reply to the question @alexpott bumped this back for. :)
Comment #36
xjmNot commit-blocking:
Why is this use statement in there twice?
We maybe don't care too much since this is provided for a unit test, but no docblock. :)
Comment #37
xjmComment #38
alexpottCommitted 5cc27d9 and pushed to 8.x. Thanks!
Fixed during commit.