Updated: Comment #N

Problem/Motivation

Throughout core, we're working to use route name/parameter pairs in the place of a path:

Entities have a uri() method that returns an array of path and options, it needs to be replaced by route name, parameters, and options.

Proposed resolution

Convert Entity::uri() to return route name and parameters in an array
Add a Entity::url() helper:

// Before
$uri = $entity->uri();
$url = url($uri['path'], $uri['options']);
// After
$url = $entity->url();

Add a secondary method for code that wants the system path in isolation, without URL processing or querystrings:

// Before
$uri = $entity->uri();
$path = $uri['path'];

// After
$path = $entity->getSystemPath();

Stop manually appending things like '/edit' to paths, making it impossible to change them sanely:

// Before
$uri = $entity->uri();
$operations['edit'] = array(
  'title' => t('Edit'),
  'path' => $uri['path'] . '/edit',
  'options' => $uri['options'],
);

// After
$operations['edit'] = array(
  'title' => t('Edit'),
) + $entity->urlInfo('edit-form');

Remaining tasks

N/A

User interface changes

N/A

API changes

EntityInterface::uriPlaceholderReplacements() is renamed to urlRouteParameters() and no longer has to wrap the keys in '{' '}'.
EntityInterface::uri() no longer returns a 'path' key, but returns 'route_name' and 'route_parameters' and has been renamed to urlInfo().
Two new methods have been added, see proposed resolution section.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Active » Needs review
FileSize
4.3 KB

This is just a testing patch. Not ready for review yet.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.45 KB

Still has a junk exception thrown.

tstoeckler’s picture

We're doing something similar in ConfigNamesMapper::getPathFromRoute() which I've never been very fond of. Would that be fixable in a similar fashion?

Might be a separate issue, don't know, but would be cool to know either way.

tim.plunkett’s picture

Title: Use UrlGenerator instead of str_replace() with uriPlaceholderReplacements() » EntityInterface::uri() should use route name and not path
Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
45.51 KB

I need to update the issue summary, but the failures are due to EntityInterface::uri() still using paths and not route names.
Marking #2165581: Get links from routes in EntityListController::getOperations as a dupe.

This patch needs a *ton* of work still, will get back to it tomorrow.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
161.63 KB

Still need to update the issue summary, but for now here's a patch.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -127,100 +120,80 @@ public function label() {
       public function uri($rel = 'canonical') {
    -    $entity_info = $this->entityInfo();
    +    if ($this->isNew()) {
    +      // Only use these defaults for a canonical link (that is, a link to self).
    +      // Other relationship types are not supported by this logic.
    +      if ($rel == 'canonical') {
    +        return array(
    +          'path' => 'entity/' . $this->entityType . '/' . $this->id(),
    +          'route_name' => '',
    +          'route_parameters' => array(),
    +          'options' => array(
    +            'entity_type' => $this->entityType,
    +            'entity' => $this,
    +          )
    +        );
    +      }
    +
    +      throw new EntityMalformedException(sprintf('The "%s" entity type has not been saved, and cannot have a URI.', $this->entityType()));
    +    }
    

    This $rel == canonical part is totally whack. Insider there $this->id() is always NULL, so the path is just 'entity/$entity_type', and is only used by CsrfTest... I have no idea what that's about.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -309,7 +309,15 @@ public function save(array $form, array &$form_state) {
       public function delete(array $form, array &$form_state) {
    -    // @todo Perform common delete operations.
    +    if ($this->entity->hasLinkTemplate('delete-form')) {
    +      $form_state['redirect_route'] = $this->entity->uri('delete-form');
    +
    +      $query = $this->getRequest()->query;
    +      if ($query->has('destination')) {
    +        $form_state['redirect_route']['options']['query']['destination'] = $query->get('destination');
    +        $query->remove('destination');
    +      }
    +    }
       }
    

    This lets us remove almost every single EntityFormController::delete override method.

  3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockListController.php
    @@ -36,10 +36,7 @@ public function buildRow(EntityInterface $entity) {
    -    // The custom block edit path does not contain '/edit'.
         if (isset($operations['edit'])) {
    -      $uri = $entity->uri();
    -      $operations['edit']['href'] = $uri['path'];
    

    No more of this needed!

  4. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.php
    @@ -130,20 +124,15 @@ function testList() {
           'delete' => array(
             'title' => t('Delete'),
    -        'href' => $uri['path'] . '/delete',
    -        'options' => $uri['options'],
             'weight' => 100,
    -      ),
    +      ) + $entity->uri('delete-form'),
    

    No more randomly appending strings to paths!

tim.plunkett’s picture

Issue summary: View changes
Related issues: +#2153891: Add a Url value object
tim.plunkett’s picture

Issue summary: View changes
tim.plunkett’s picture

Issue tags: -Needs issue summary update
FileSize
204.35 KB

Forgot to include the getInternalPath() part.

tim.plunkett’s picture

FileSize
205.75 KB
1.4 KB

Forgot to fix ImageFieldDisplayTest.

tim.plunkett’s picture

FileSize
206.13 KB
2.44 KB

Missed a spot!
Also cleaned up a couple docblocks.

tim.plunkett’s picture

FileSize
216.55 KB
11.84 KB

I realized the weird hack I left in the code for REST stuff was no longer needed. Removed that.

This now obsoletes #2107581: Configurables uri() always returns 'edit-form' link, because it contains the fix by necessity, and I've added the test coverage from that issue, and expanded it.

Leaving that open for now though.

This is now ready for full review.

tim.plunkett’s picture

Issue summary: View changes
tim.plunkett’s picture

Issue summary: View changes
tim.plunkett’s picture

Issue summary: View changes
FileSize
228.89 KB

After a long discussion with @sun and @Crell, I've renamed the methods.
uri() is now urlInfo().
url() stays.
getInternalPath() is now getSystemPath(), to mimic the naming of AliasManagerInterface and _system_path

tim.plunkett’s picture

FileSize
229.04 KB

Status: Needs review » Needs work

The last submitted patch, 22: entity-uri-2167641-22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

22: entity-uri-2167641-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: entity-uri-2167641-22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

22: entity-uri-2167641-22.patch queued for re-testing.

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -207,8 +207,22 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +  public function urlInfo($rel = 'edit-form') {
    +    return parent::urlInfo($rel);
    +  }
    

    Classy.

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -127,100 +119,71 @@ public function label() {
    +    else {
    +      $bundle = $this->bundle();
    +      // A bundle-specific callback takes precedence over the generic one for
    +      // the entity type.
    +      $bundles = \Drupal::entityManager()->getBundleInfo($this->entityType);
    +      if (isset($bundles[$bundle]['uri_callback'])) {
    +        $uri_callback = $bundles[$bundle]['uri_callback'];
           }
    -      catch (RouteNotFoundException $e) {
    -        // Fall back to a non-template-based URI.
    +      elseif ($entity_uri_callback = $this->entityInfo()->getUriCallback()) {
    +        $uri_callback = $entity_uri_callback;
           }
    

    How will #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method impact this? The url callback function is problematic anyway at this point.

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -127,100 +119,71 @@ public function label() {
    +      // @todo Convert to is_callable() and call_user_func().
    +      if (isset($uri_callback) && function_exists($uri_callback)) {
    +        $uri = $uri_callback($this);
    +      }
    

    Is there a reason to not todo this while we're already modifying this code? What is this todo blocked on?

  4. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,36 @@ protected function linkTemplates() {
    +  public function url($rel = 'canonical', $options = array()) {
    +    if ($this->isNew() || !$uri = $this->urlInfo($rel)) {
    +      return '';
    +    }
    

    urlInfo() has the same check, but throws an exception. This returns empty string. If there's a reason for them to be different then it should be documented.

  5. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,36 @@ protected function linkTemplates() {
    -  protected function uriPlaceholderReplacements() {
    -    if (empty($this->uriPlaceholderReplacements)) {
    -      $this->uriPlaceholderReplacements = array(
    -        '{entityType}' => $this->entityType(),
    -        '{bundle}' => $this->bundle(),
    -        '{id}' => $this->id(),
    -        '{uuid}' => $this->uuid(),
    -        '{' . $this->entityType() . '}' => $this->id(),
    -      );
    

    Please don't delete this method. It will be needed once we move back to URI templates on entities. (I know you disagree with that; that discussion belongs in the other issue, not here. Leave it for now, please, so we don't have to deal with that here.)

  6. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -97,11 +97,71 @@ public function label();
    +  /**
    +   * Returns the URL for this entity.
    +   *
    +   * @param string $rel
    +   *   The link relationship type, for example: canonical or edit-form.
    +   * @param array $options
    +   *   See \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute() for
    +   *   the available options.
    +   *
    +   * @return string
    +   *   The URL for this entity.
    +   */
    +  public function url($rel = 'canonical', $options = array());
    +
    +  /**
    +   * Returns the internal path for this entity.
    +   *
    +   * The self::url() method will return the full path including any prefixes,
    +   * fragments, or query strings. This path does not include those.
    +   *
    +   * @param string $rel
    +   *   The link relationship type, for example: canonical or edit-form.
    +   *
    +   * @return string
    +   *   The internal path for this entity.
    +   */
    +  public function getSystemPath($rel = 'canonical');
    

    The distinction between these two methods is not 100% clear. url() also includes aliasing, for instance.

    perhaps url() should be listed as "Returns the public URL for this entity", vis, the one that gets displayed to the world. Then note the lack of aliasing in getSystemPath().

    Also, I don't think we've referred to other methods in the same class as self::whatever() before in documentation. I'm not against it, but I haven't seen it as a convention yet.

  7. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
    @@ -93,24 +93,18 @@ protected function getLabel(EntityInterface $entity) {
    +      ) + $entity->urlInfo('edit-form');
    

    This is so much closer to the world I'm looking for. :-) (Still not 100% there, but way closer.)

  8. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
    @@ -28,6 +28,12 @@
    + *   links = {
    + *     "canonical" = "aggregator.feed_view",
    + *     "edit-form" = "aggregator.feed_configure",
    + *     "delete-form" = "aggregator.feed_delete",
    + *     "edit-form" = "aggregator.feed_configure",
    + *   },
    

    While I'd still prefer these were URI templates, I like that the links array is getting more widely used with more entities.

I didn't go over the rest of the patch in detail, as it's just applying the above patterns a lot.

tim.plunkett’s picture

Issue tags: +Avoid commit conflicts
FileSize
1.09 KB
229.26 KB

2) Well that issue is about completely deleting this code, so it doesn't matter that I move it. (It looks like I'm changing it, but if you apply the patch and use -w you'll see I'm mostly outdenting)

3) It just wasn't in scope, I didn't want to add test coverage, and #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method plans to delete it anyway. I didn't know it was, but I like that. This patch should actually make that issue easier.

4) Will add docs, but returning an empty string meets the expectation of the callers.

5) I'm not going to just leave dead code around. This is a self-contained and simple method that can be readded if we decide to. And I didn't just delete it, it was replaced by urlRouteParameters(), which is essentially the same thing without the wrapping {...}

6) I'll make that more explicit. And yes, we do use self:: for inline docs now.

Status: Needs review » Needs work

The last submitted patch, 28: uri-2167641-28.patch, failed testing.

The last submitted patch, 28: uri-2167641-28.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
228.37 KB

Heh, the hook_menu_links_default patch also added a vocabularyTitle title callback, just to different parts of the same file :)

tim.plunkett’s picture

FileSize
228.07 KB

#1987396: Refactor & Clean-up comment.module theme functions moved a bunch of code I was changing, but it was easy enough to fix...

#2181045: Add a title callback for the term overview page went in to prevent further clashes with the hook_menu_link_defaults issue.

dawehner’s picture

First bit before the browser stopped to work.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -207,8 +207,22 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +  public function urlInfo($rel = 'edit-form') {
    +    return parent::urlInfo($rel);
    +  }
    

    Should we add a follow up to not provide some default? It feels like of wrong to do this.

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -127,100 +119,71 @@ public function label() {
    +    if ($this->isNew()) {
    +      throw new EntityMalformedException(sprintf('The "%s" entity type has not been saved, and cannot have a URI.', $this->entityType()));
    +    }
    

    This seems to be something new, do we need this?

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,38 @@ protected function linkTemplates() {
    +    // While self::urlInfo() will throw an exception if the entity is new,
    +    // the expected result for a URL is always a string.
    

    Can we also explain here why this is expected?

  4. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,38 @@ protected function linkTemplates() {
    +    if ($rel == 'admin-form' && $this->entityInfo()->getBundleEntityType() != 'bundle') {
    +      $uri_route_parameters[$this->entityInfo()->getBundleEntityType()] = $this->bundle();
         }
    

    It is a bit odd to hardcode that, but I guess there aren't any better fix.

  5. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    index 2ea0fbe..8a027ed 100644
    --- a/core/lib/Drupal/Core/Entity/EntityFormController.php
    
    --- a/core/lib/Drupal/Core/Entity/EntityFormController.php
    +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    
    +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -309,7 +309,15 @@ public function save(array $form, array &$form_state) {
    
    @@ -309,7 +309,15 @@ public function save(array $form, array &$form_state) {
        *   A reference to a keyed array containing the current state of the form.
        */
       public function delete(array $form, array &$form_state) {
    -    // @todo Perform common delete operations.
    +    if ($this->entity->hasLinkTemplate('delete-form')) {
    +      $form_state['redirect_route'] = $this->entity->urlInfo('delete-form');
    +
    +      $query = $this->getRequest()->query;
    +      if ($query->has('destination')) {
    +        $form_state['redirect_route']['options']['query']['destination'] = $query->get('destination');
    +        $query->remove('destination');
    +      }
    +    }
       }
    

    Is this required as part of the patch?

Crell’s picture

#33.1: That's actually quite smart. The base class has a default of "canonical". For config entities, though, there is no "view" page. I suppose technically we could make canonical and edit-form the same URI, but in most cases if you want "the link I should click to get to the config entity", you probably mean the edit-form. So we either do this, or set edit-form and canonical to be the same path. I suppose I could be happy with either. It's already doing the edit-form version in HEAD right now, though.

tim.plunkett’s picture

1) I think we should either remove the default everywhere or leave it here. Entity has a default of 'canonical', but config entities don't have a canonical link template...

2) We absolutely need this now. Previously, we would just manually build up a path, and you would end up with 'entity/' . $this->entityType for non-saved entities. That doesn't work now with the route-based approach.

3) It is expected by the calling code we have in core. They just want to know what string to use, and an empty string is fine. Forcing them to do the isNew check, or worse catching the exception, is not expected. Unlike when calling urlInfo() yourself, when you need to introspect the actual route name and params.

4) Yeah, there wasn't much else to do here.

5) I was going through and fixing each entity form's delete(), until I realized they were all the same when there was a delete-form link template.
Forgetting to write this delete() method has burned me on every ConfigEntity I've written, so I'm very happy about this being in.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
228.08 KB

Conflicted with #2164367: Rebuild router as few times as possible per request on patch context.

EDIT: #2182899: Use the --3way option with git apply would be a way to prevent some of these rerolls.

Berdir’s picture

Looks great to me overall. Two warnings:
- I did not read anything in this issue at all, I just read through the patch. If anything was discussed already, feel free to point me to the discussion or ignore it :)
- Lots of small stuff below, but I did went through the whole patch and wrote down my thoughts. Sometimes I ask questions and then answer them myself later on, so read everything before you respond :)

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -207,8 +207,22 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
        * {@inheritdoc}
        */
    -  public function uri() {
    -    return parent::uri('edit-form');
    +  public function urlInfo($rel = 'edit-form') {
    +    return parent::urlInfo($rel);
    +  }
    +
    

    Thank you! I was trying to suggest that in issue that changed that but it was ignored ;)

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -127,100 +119,71 @@ public function label() {
     
         // Pass the entity data to url() so that alter functions do not need to
         // look up this entity again.
    +    $uri['options']['entity_type'] = $this->entityType;
         $uri['options']['entity'] = $this;
    +
    

    I think entity_type was removed explicitly because you can access it through 'entity']->getEntityTypeId.

    This and other parts will also conflict with the getEntityTypeId() issue.

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,38 @@ protected function linkTemplates() {
    +  public function url($rel = 'canonical', $options = array()) {
    +    // While self::urlInfo() will throw an exception if the entity is new,
    +    // the expected result for a URL is always a string.
    +    if ($this->isNew() || !$uri = $this->urlInfo($rel)) {
    +      return '';
    +    }
    

    I know there was an issue with the idea to enforce that entities always have a URL, that's where the default entity/ stuff was introduced. No longer possible with routes obviously, wondering if that could be a problem somewhere, e.g. for rest or so?

  4. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,38 @@ protected function linkTemplates() {
        *
    +   * @param string $rel
    +   *   The link relationship type, for example: canonical or edit-form.
    +   *
    

    Not sure, but the : makes it sound like those two are the only options, is that necessary?

  5. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -231,26 +194,38 @@ protected function linkTemplates() {
    +  protected function urlRouteParameters($rel) {
    +    $uri_route_parameters[$this->entityType()] = $this->id();
    +    if ($rel == 'admin-form' && $this->entityInfo()->getBundleEntityType() != 'bundle') {
    +      $uri_route_parameters[$this->entityInfo()->getBundleEntityType()] = $this->bundle();
         }
    

    Not sure if I understand what this is checking exactly, a comment might help?

  6. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -97,11 +97,71 @@ public function label();
        *   of the entity, and matching the signature of url().
        */
    -  public function uri();
    +  public function urlInfo($rel = 'canonical');
    +
    

    Ỳay, more clean-up of only partially supporting the $rel argument :)

  7. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -97,11 +97,71 @@ public function label();
    +   *
    +   * The self::url() method will return the full path including any prefixes,
    +   * fragments, or query strings. This path does not include those.
    

    Not sure, but just "self::url() will return.. " sounds better to me.

  8. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -273,8 +273,7 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
             }
             else {
    -          $uri = $entity->uri();
    -          $destination = array('destination' => $uri['path'] . '#comment-form');
    +          $destination = array('destination' => $entity->getSystemPath() . '#comment-form');
             }
    

    Is the mix of system path and #fragement for a destination URL really correct?

  9. +++ b/core/modules/config_translation/config_translation.module
    @@ -154,10 +154,10 @@ function config_translation_config_translation_info(&$info) {
     function config_translation_entity_operation_alter(array &$operations, EntityInterface $entity) {
       if (\Drupal::currentUser()->hasPermission('translate configuration')) {
    -    $uri = $entity->uri();
    +    $uri = $entity->urlInfo();
         $operations['translate'] = array(
           'title' => t('Translate'),
    -      'href' => $uri['path'] . '/translate',
    +      'href' => $entity->getSystemPath() . '/translate',
           'options' => $uri['options'],
           'weight' => 50,
    

    Mixing system path and urlInfo() options seems a bit strange, are you sure this is compatible?

  10. +++ b/core/modules/field_ui/field_ui.module
    @@ -122,6 +122,15 @@ function field_ui_entity_info(&$entity_info) {
    +
    +  foreach ($entity_info as $info) {
    +    if ($bundle = $info->getBundleOf()) {
    +      $info
    +        ->setLinkTemplate('field_ui-fields', "field_ui.overview_$bundle")
    +        ->setLinkTemplate('field_ui-form-display', "field_ui.form_display_overview_$bundle")
    +        ->setLinkTemplate('field_ui-display', "field_ui.display_overview_$bundle");
    +    }
    +  }
    

    This is nice, so every entity has a link template for this.

    Just wondering, all link templates are I think right now added to the rest output of an entity, do we really want all that stuff there?

  11. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.php
    @@ -273,7 +273,7 @@ function testSingleViewMode() {
     
         // This may not trigger a notice when 'view_modes_custom' isn't available.
    -    $this->drupalPostForm('admin/structure/taxonomy/manage/' . $this->vocabulary . '/display', array(), t('Save'));
    +    $this->drupalPostForm('admin/structure/taxonomy/manage/' . $this->vocabulary . '/overview/display', array(), t('Save'));
       }
    

    Can see that change repeated below, why does that change here?

  12. +++ b/core/modules/filter/lib/Drupal/filter/Entity/FilterFormat.php
    @@ -39,7 +39,8 @@
      *     "status" = "status"
      *   },
      *   links = {
    - *     "edit-form" = "filter.format_edit"
    + *     "edit-form" = "filter.format_edit",
    + *     "disable" = "filter.admin_disable"
      *   }
    

    It's a good thing that we wrote quite a few tests for all these links as we broke it multiple times already.

    Might be useful to go through all entity lists manually, if this didn't happen already?

  13. +++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
    @@ -102,21 +102,12 @@ public function save(array $form, array &$form_state) {
    +    $query = $this->getRequest()->query;
    +    if ($query->has('destination')) {
    +      $form_state['redirect_route']['options']['query']['destination'] = $query->get('destination');
         }
    

    The default implementation renmoves the destination after using it, this doesn't, why?

  14. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
    @@ -284,7 +284,7 @@ private function doAdminTests($user) {
         // Test vocabulary form alterations.
    -    $this->drupalGet('admin/structure/taxonomy/manage/forums/edit');
    +    $this->drupalGet('admin/structure/taxonomy/manage/forums');
         $this->assertFieldByName('op', t('Save'), 'Save button found.');
    

    Ah, I think I know what's going on, overview was the default tab before and now it's edit?

    I don't get why, though?

  15. +++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizeTest.php
    @@ -21,6 +21,15 @@ public static function getInfo() {
    +   * {@inheritdoc}
    +   */
    +  function setUp() {
    +    parent::setUp();
    +
    +    \Drupal::service('router.builder')->rebuild();
    +  }
    +
    

    Just wondering if this is still necessary after the router rebuild stuff went in?

  16. +++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -95,7 +95,7 @@ public function viewElements(FieldItemListInterface $items) {
         $image_link_setting = $this->getSetting('image_link');
         // Check if the formatter involves a link.
         if ($image_link_setting == 'content') {
    -      $uri = $items->getEntity()->uri();
    +      $uri['path'] = $items->getEntity()->getSystemPath();
         }
         elseif ($image_link_setting == 'file') {
    

    Unlike one of the examples above, this does not add the options, which were there before...

  17. +++ b/core/modules/language/lib/Drupal/language/LanguageListController.php
    @@ -45,16 +45,6 @@ public function getOperations(EntityInterface $entity) {
         // Deleting the site default language is not allowed.
         if ($entity->id() == $default->id) {
           unset($operations['delete']);
    

    I know, out of context, just wondering, but this should be handled through the access controller, not by altering the operations..

  18. +++ b/core/modules/node/node.module
    @@ -647,8 +650,7 @@ function template_preprocess_node(&$variables) {
     
    -  $uri = $node->uri();
    -  $variables['node_url']  = url($uri['path'], $uri['options']);
    +  $variables['node_url'] = empty($node->in_preview) ? $node->url() : '';
       $variables['label'] = $variables['elements']['title'];
    

    Makes sense, just wondering why it's necessary in the context of this issue? Given that url() returns '' for new entities anyway.

  19. +++ b/core/modules/search/lib/Drupal/search/Entity/SearchPage.php
    @@ -33,7 +33,11 @@
    + *     "delete-form" = "search.delete",
    + *     "enable" = "search.enable",
    + *     "disable" = "search.disable",
    

    Not a pattern that you are introducing here, but enable/disable are just as form-y as delete, no?

  20. +++ b/core/modules/system/entity.api.php
    @@ -699,10 +699,9 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
    -  $uri = $entity->uri();
       $operations['translate'] = array(
         'title' => t('Translate'),
    -    'href' => $uri['path'] . '/translate',
    +    'href' => $entity->getSystemPath() . '/translate',
         'weight' => 50,
    

    A follow-up to use the same pattern for translate routes as we do for field ui would be nice?

  21. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -39,10 +39,10 @@ taxonomy.vocabulary_add:
     taxonomy.vocabulary_edit:
    -  path: '/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/edit'
    +  path: '/admin/structure/taxonomy/manage/{taxonomy_vocabulary}'
       defaults:
         _entity_form: 'taxonomy_vocabulary.default'
    -    _title: 'Edit vocabulary'
    +    _title_callback: '\Drupal\taxonomy\Controller\TaxonomyController::vocabularyTitle'
       requirements:
    

    Yep, as I guess above.

    Are the title changes necessary for tests?

    And still wondering why the default tabs are switched for vocabulary and I think shortcut set above was similar?

  22. +++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityCallbacksTest.php
    @@ -21,6 +21,11 @@ class UserEntityCallbacksTest extends WebTestBase {
     
    +  /**
    +   * @var \Drupal\Core\Entity\EntityInterface
    +   */
    +  protected $account;
    +
    

    As you add a type hint, shouldn't you make that a UserInterface while you're at it?

tim.plunkett’s picture

FileSize
6.31 KB
228.63 KB

1) You're welcome :)

2) In HEAD the entity_type is still added in only part of the code path. I didn't want to remove it here.

3) #2019123: Use the same canonical URI paths as for HTML routes is addressing this

4) That's copy/pasted.

5) Added a comment. This is for getting to /admin/structure/types/manage/article for a node entity.

7) Fixed

8) and 9) This is absolutely not okay, but it is done in HEAD, and very out of scope here. This is one of the few offenders outside of the translation modules and Views.

10) I'm not sure if we want that, but we're not changing anything, just expanding the usage of it. Might be worth a REST follow-up.

11) So all config entities with a UI have an edit-form. Except in order to trick the UI, vocabulary defines its overview page as its edit form, which is just not true. In order to correctly identify the correct edit-form while maintaining the expected taxonomy UI, we need to massage these paths.

12) I manually went through each entity type myself. More had test coverage than I thought, which was nice.

13) I believe this was just an oversight on my part. If something fails, we'll know why and can add a comment.

14) See point 11. This is the opposite of adding /overview/ to places.

15) Yes this is still necessary. That issue did not help DUTB tests at all, because nothing ever sets the "if needed" flag.

16) Turns out I had half of the right code in PictureFormatter, and the other in ImageFormatter. Fixed.

17) It would be nice...

18) This shouldn't be needed anymore, it was before some refactoring to avoid the exception in urlInfo(). Fixed here and in EntityNormalizer.

19) No, these (and Views UI) just do the enabling/disabling and redirect.

20) See points 8/9

21) The title change is needed to maintain the expected UI and breadcrumbs

22) Sure!

tim.plunkett’s picture

FileSize
228.62 KB

#2167109: Remove Variable subsystem changed the old EntityUriTest web test, which is being removed here.

tim.plunkett’s picture

40: uri-2167641-40.patch queued for re-testing.

tim.plunkett’s picture

Priority: Major » Critical

Someone pointed out that we shouldn't have a non-critical "Avoid commit conflicts".

If we don't finish the conversion from paths to route names before release, all of that time was wasted. Route names are useless if anything in code refers to a path.

tim.plunkett’s picture

FileSize
228.62 KB
tim.plunkett’s picture

FileSize
228.71 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this.

alexpott’s picture

Title: EntityInterface::uri() should use route name and not path » Change record: EntityInterface::uri() should use route name and not path
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: -Avoid commit conflicts +Needs change record

Committed 76bcb27 and pushed to 8.x. Thanks!

alexpott’s picture

xjm’s picture

Issue tags: +Missing change record
aspilicious’s picture

+   * URI templates might be set in the links array in an annotation, for
+   * example:
+   * @code
+   * links = {
+   *   "canonical" = "/node/{node}",
+   *   "edit-form" = "/node/{node}/edit",
+   *   "version-history" = "/node/{node}/revisions"
+   * }
+   * @endcode

I think these examples should be routified now?
Probably needs a small followup for this...

Xano’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
ann b’s picture

Assigned: Unassigned » ann b

Hi,

I'm going to attempt to write the change record for this issue. I am fairly new to drupal though. If this change record should really be handled by a senior developer, just let me know. I'll change the status back to unassigned.

Ann

ann b’s picture

Assigned: ann b » Unassigned
Status: Active » Needs review
Issue tags: -Needs change record, -Missing change record

I have created a change record. Let's see how it goes. Please see https://drupal.org/node/2221879.

tim.plunkett’s picture

Since #2215961: Entity::urlInfo() should return \Drupal\Core\Url went in, all of this changed. I used @ann b's work as a base, can someone review?

tim.plunkett’s picture

Priority: Major » Critical
Issue tags: +Missing change record

There is a draft, but its "missing"

xjm’s picture

Issue tags: +beta blocker
pwolanin’s picture

Title: Change record: EntityInterface::uri() should use route name and not path » EntityInterface::uri() should use route name and not path
Status: Needs review » Fixed
Issue tags: -Missing change record, -beta blocker

The change record seems to be published? It looks fine, so marking this fixed.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Priority: Major » Critical

Actually it was critical; I was just confused by the CR noise.