Updated: Comment #N

Problem/Motivation

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()

Proposed resolution

Remove the completely useless array typehint.

Remaining tasks

User interface changes

N/A

API changes

The change to ContainerFactoryPluginInterface::create() is the only API break.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
119.62 KB
Xano’s picture

Doesn'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\Action returns them. Shouldn't the action base class contain keep its array type hint because of that?

neclimdul’s picture

On 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.

tim.plunkett’s picture

Typehints 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.

tim.plunkett’s picture

Issue tags: +beta target
FileSize
127.98 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 5: plugin-2170775-5.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
143.45 KB
14.7 KB

Forgot to rebase after migrate went in.

dawehner’s picture

I wonder whether we really want to remote it in all the places, we know, we don't use objects.

Xano’s picture

FileSize
143.64 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 9: drupal_2170775_9.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
143.62 KB
2.59 KB

Status: Needs review » Needs work

The last submitted patch, 11: drupal_2170775_11.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
Xano’s picture

11: drupal_2170775_11.patch queued for re-testing.

martin107’s picture

FileSize
143.42 KB

Patch no longer applied, so rerolled.

BTW +1 on this issue it makes perfect sense to me.

Status: Needs review » Needs work

The last submitted patch, 15: drupal_2170775_15.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
144.72 KB
1.3 KB

Ok this next patch works locally..

SystemMenuBlock.php was not mentioned in the original patch, and so was a recent addition from HEAD.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in while it is green

webchick’s picture

Priority: Major » Normal
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit fc6b814 on 8.x by webchick:
    Issue #2170775 by tim.plunkett, martin107, Xano: Remove array typehint...
dawehner’s picture

I do consider this as major, because it is part of the main API everyone uses

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.