EntityRouteEnhancer::enhance() has a bit set of ifs/elseifs as it tries to figure out whether the incoming data has one of _entity_form / _entity_list / _entity_view set.

This would be a lot easier to read as a switch.

The switch (TRUE) construction can be used here:

switch (TRUE) {
  case !empty($defaults['_entity_form']):
 // etc
}

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Nothing is broken, so its a task
Issue priority Normal, because the amount of impact is not that high, but ensuring that quite an important class is actually readable is quite a huge win.
Disruption No disruption, for non given input the output will change.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alumei’s picture

Status: Active » Needs review
FileSize
5.95 KB

Something like this?

Grimreaper’s picture

Status: Needs review » Reviewed & tested by the community

Hello,

I just review the code. It looks good to me.

Is there an impact on the interface to test ?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

In what way is this easier to read or comprehend? I don't think I've ever seen switch(TRUE) used like that in any language.

joachim’s picture

I've seen switch(TRUE) used quite a few times. I find it's easier to read, though it is a bit surprising at first.

msonnabaum’s picture

Status: Needs review » Needs work

This is trying to solve the wrong problem. If you want it to be more readable, just decompose the method into smaller chunks, so that all you're left with is the logic, which then becomes much easier to read.

The switch statement does not improve readability.

fran seva’s picture

@msonnabaum I think the same but also think the use of switch can also improve the readability

What do you think about this chunks of code? I've separated the last ifelse statement in a new function to reduce the nested "if's".
I'm not sure if this is a correct practice in Drupal core development.

public function enhance(array $defaults, Request $request) {
    if (empty($defaults['_content'])) {
      if (!empty($defaults['_entity_form'])) {
        $wrapper = new HtmlEntityFormController($this->resolver, $this->manager, $this->formBuilder, $defaults['_entity_form']);
        $defaults['_content'] = array($wrapper, 'getContentResult');
      }
      elseif (!empty($defaults['_entity_list'])) {
        $defaults['_content'] = '\Drupal\Core\Entity\Controller\EntityListController::listing';
        $defaults['entity_type'] = $defaults['_entity_list'];
        unset($defaults['_entity_list']);
      }
      elseif (!empty($defaults['_entity_view'])) {
       $defaults['_content'] = '\Drupal\Core\Entity\Controller\EntityViewController::view';

       $defaults = $this->enhanceChunk($defaults, $request);
      }
    }
    return $defaults;
  }

  protected function enhanceChunk(array $defaults, Request $request) {


    switch (TRUE) {
      case strpos($defaults['_entity_view'], '.') !== FALSE :
        // The _entity_view entry is of the form entity_type.view_mode.
        list($entity_type, $view_mode) = explode('.', $defaults['_entity_view']);
        $defaults['view_mode'] = $view_mode;
        break;
      default:
        // Only the entity type is nominated, the view mode will use the
        // default.
        $entity_type = $defaults['_entity_view'];
        break;
    }

    switch (TRUE) {
      case !empty($defaults[$entity_type]) :
        $defaults['_entity'] = &$defaults[$entity_type];
        break;
      default:
        // The entity is not keyed by its entity_type. Attempt to find it
        // using a converter.
        $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
        if ($route && is_object($route)) {
          $options = $route->getOptions();
          if (isset($options['parameters'])) {
            foreach ($options['parameters'] as $name => $details) {
              if (!empty($details['type'])) {
                $type = $details['type'];
                // Type is of the form entity:{entity_type}.
                $parameter_entity_type = substr($type, strlen('entity:'));
                if ($entity_type == $parameter_entity_type) {
                  // We have the matching entity type. Set the '_entity' key
                  // to point to this named placeholder. The entity in this
                  // position is the one being rendered.
                  $defaults['_entity'] = &$defaults[$name];
                }
              }
            }
          }
          else {
            throw new \RuntimeException(sprintf('Failed to find entity of type %s in route named %s', $entity_type, $defaults[RouteObjectInterface::ROUTE_NAME]));
          }
        }
        else {
          throw new \RuntimeException(sprintf('Failed to find entity of type %s in route named %s', $entity_type, $defaults[RouteObjectInterface::ROUTE_NAME]));
        }
        unset($defaults['_entity_view']);
    }
    unset($defaults['_entity_view']);
  }

Is this the way you are thinking?

fran seva’s picture

Code refactor propousal.
phpcs shows:phpcs: Protected method name "EntityRouteEnhancer::enhance_route_entity_view" is not in lowerCamel format, it must not contain underscores (at line 95)
But I don't know if this is the right coding standard.I've seen another issue where it was said to not use lowerCamel #1533250

cs_shadow’s picture

Status: Needs work » Needs review
fran seva’s picture

In [1] @TravisCarden confirm that the phpcs Drupal standard is correct. I will update the code and create a new patch ASAP ...this night :P

[1] https://drupal.org/comment/8664591#comment-8664591

fran seva’s picture

Patch with my proposal without Code Sniffer errors.

dawehner’s picture

Status: Needs review » Needs work

While I am not convinced that the switch is better than than just a bunch of if/elseif/ statements moving the big hunk into a separate method is certainly a win.

+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
@@ -62,64 +62,81 @@ public function __construct(ControllerResolverInterface $resolver, EntityManager
+  /**
+   * {@inheritdoc}
+   */
+  protected function enhanceRouteEntityView(array &$defaults, Request $request) {

Nope, {@inheritdoc} does not solve all documentation for you

tim.plunkett’s picture

Title: change EntityRouteEnhancer::enhance() to use switch rather than a big if/elseif » Change EntityRouteEnhancer::enhance() to use protected methods instead of one giant method

Let's keep the improvements and ditch the wonky switch.

cmanalansan’s picture

Triaged as Novice:

Let's keep the improvements and ditch the wonky switch.

Latest patch here:
https://www.drupal.org/node/2218145#comment-8673313

mglaman’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

Here is an updated patch which moves each check's modifications to the defaults into its own protected method, per #12

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -23,59 +23,13 @@ class EntityRouteEnhancer implements RouteEnhancerInterface {
    +        $defaults = $this->ehnanceEntityForm($defaults, $request);
    ...
    +        $defaults = $this->ehnanceEntityList($defaults, $request);
    

    it should be enhance or ehnance ...

  2. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -92,4 +46,105 @@ public function applies(Route $route) {
    +   * @param Request $request
    ...
    +   * @param Request $request
    ...
    +   * @param Request $request
    

    The doc standard is using full qualified class names (FQCN)

  3. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -92,4 +46,105 @@ public function applies(Route $route) {
    +  protected function ehnanceEntityView(array $defaults, Request $request) {
    ...
    +          throw new \RuntimeException(sprintf('Failed to find entity of type %s in route named %s', $entity_type, $defaults[RouteObjectInterface::ROUTE_NAME]));
    ...
    +        throw new \RuntimeException(sprintf('Failed to find entity of type %s in route named %s', $entity_type, $defaults[RouteObjectInterface::ROUTE_NAME]));
    

    This method throws exceptions so we should better document with @throws

mglaman’s picture

Here is updated patch per review in #15

mglaman’s picture

FileSize
6.98 KB

Apparently the word enhance is impossible for me to type properly. Interdiff from #16 still relevant, this just fixes typos.

The last submitted patch, 16: interdiff-2218145-14-16.diff, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks great for me now!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation

This needs a beta evaluation - I agree that the proposed change makes the class much more readable and therefore less fragile when making change. So I think this is permittable under committer discretion of reducing fragility.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs beta evaluation

Sure

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2f853b1 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 2f853b1 on 8.0.x
    Issue #2218145 by mglaman, fran seva, Alumei, dawehner: Change...

Status: Fixed » Closed (fixed)

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