Follow-up to #1906810: Require type hints for automatic entity upcasting

Problem/Motivation

Commit from #1906810: Require type hints for automatic entity upcasting breaks possibility to pass function name to '_content' property of the route.
For example :

test.route:
  path: 'some/path'
  defaults:
    _title: 'Title'
    _content: 'functionName'

From here : This could be a PHP 4 style function, but best practice in Drupal 8 is to use a controller class.

It is all because of EntityResolverManager::setRouteOptions method. If u look at it you will see that $this->getController
call should return array with controller instance and method name as its values. (docblock comment)

Next. Looking at the getController method.

    if (isset($defaults['_content'])) {
      $controller = $this->controllerResolver->getControllerFromDefinition($defaults['_content']);
    }

And here is it. getControllerFromDefinition method can return just callback function name instead of array.
This scenario is not covered in implementation and u will get en error : "Argument 1 passed to Drupal\Core\Entity\EntityResolverManager::setParametersFromReflection() must be of the type array, string given, called in /home/yarik/www/drupal8/dev/core/lib/Drupal/Core/Entity/EntityResolverManager.php on line 188"

Proposed resolution

Fix it.

Comments

undertext’s picture

Status: Active » Needs review
StatusFileSize
new1.56 KB

Maybe something like this patch.

dawehner’s picture

Issue tags: +WSCCI, +Needs tests

Let's ensure that we continue to have full test coverage for this class ...

m1r1k’s picture

Issue tags: +#ams2014contest
jibran’s picture

It also needs test only patch.

+++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
@@ -94,21 +94,28 @@ protected function getController(array $defaults) {
+   * @param  $controller

typehint missing.

dawehner’s picture

Status: Needs review » Needs work

So, needs work.

damiankloip’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review
Issue tags: -
StatusFileSize
new7.36 KB
new8.93 KB
new8.04 KB

Yep, let's add this case to the test coverage too.

damiankloip’s picture

Priority: Normal » Major

Bumping as it doesn't look good if you just enter a function name and get a nice error like that.

The last submitted patch, 6: 2321393-6-tests-only-FAIL.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice!!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6d8b0b8 and pushed to 8.0.x. Thanks!

  • alexpott committed 6d8b0b8 on 8.0.x
    Issue #2321393 by damiankloip, undertext: Fixed Unable to pass function...

Status: Fixed » Closed (fixed)

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