Updated: Comment #N
Starting with #2005716: Promote EntityType to a domain object we no longer require plugin definitions to be arrays. However, the following methods have array typehints on the plugin definition.
ContainerFactoryPluginInterface::create()
PluginBase::__construct()
ContextAwarePluginBase::__construct()
DefaultFactory::getPluginClass()
Remove the completely useless array typehint.
N/A
The change to ContainerFactoryPluginInterface::create() is the only API break.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | interdiff-15.17.txt | 1.3 KB | martin107 |
| #17 | drupal_2170775_17.patch | 144.72 KB | martin107 |
| #15 | drupal_2170775_15.patch | 143.42 KB | martin107 |
| #11 | interdiff.txt | 2.59 KB | xano |
| #11 | drupal_2170775_11.patch | 143.62 KB | xano |
Comments
Comment #1
tim.plunkettComment #2
xanoDoesn't this depend on what the plugin-type-specific managers expect? The plugin API itself should not depend on arrays, but (to take actions as an example), actions still have array definitions, because
\Drupal\Core\Annotation\Actionreturns them. Shouldn't the action base class contain keep its array type hint because of that?Comment #3
neclimdulOn the one hand, I feel like it'd be great to have a better type hint. On the other hand, I think this is just a case were we can admit we're working in a loosely typed scripting language and let it pass. Maybe further down the line as we feel out the impacts of definition objects we can enforce some sort of PluginDefinitionInterface if its appropriate.
This is technically a signature change so I don't know if it needs any approval but its not really affecting anything so +1 RTBC.
Comment #4
tim.plunkettTypehints are useful when they give you more information. Typehinting with interfaces tells you what methods you can call on an object. But typehinting with an array is all but useless.
We could leave the array typehint on the __construct of the concrete classes and just fix the base classes, it doesn't matter too much to me.
Comment #5
tim.plunkettReroll
Comment #7
tim.plunkettForgot to rebase after migrate went in.
Comment #8
dawehnerI wonder whether we really want to remote it in all the places, we know, we don't use objects.
Comment #9
xanoRe-roll.
Comment #11
xanoComment #13
xanoComment #14
xano11: drupal_2170775_11.patch queued for re-testing.
Comment #15
martin107 commentedPatch no longer applied, so rerolled.
BTW +1 on this issue it makes perfect sense to me.
Comment #17
martin107 commentedOk this next patch works locally..
SystemMenuBlock.php was not mentioned in the original patch, and so was a recent addition from HEAD.
Comment #18
dawehnerLet's get it in while it is green
Comment #19
webchickComment #20
webchickCommitted and pushed to 8.x. Thanks!
Comment #22
dawehnerI do consider this as major, because it is part of the main API everyone uses