Updated as of #18

Problem/Motivation

  1. You cannot tell the URI pattern for a config entity, eg. to autogenerate subtabs/routes for it
  2. You need to copy-paste lots of code to avoid getting crappy entity/view/12 type URLs

Proposed resolution

  • Use the links list pattern as is with content entities in a similar way for config entities
      *   links = {
      *     "edit-form" = "admin/structure/block/custom-blocks/manage/{custom_block_type}"
      *   }
    
  • Because config entities uri() method does not take a $rel like EntityNG does, just use 'edit-form' (given config entity paths point to edit pages)
    +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -188,4 +188,11 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +  public function uri() {
    +    return parent::uri('edit-form');
    +  }
    

Remaining tasks

  • (done) Apply to all config entity types (See #14)
  • Update docs for config entity annotations
  • make an issue to move this to route_name instead of paths (See #3)

User interface changes

None.

API changes

Support links list in config entity annotations.

Related issues

The config mapping code added in #1952394: Add configuration translation user interface module in core would be greatly simplified by this, and this will help avoid problems like #2085603: Admin path of blocks changed, menus not prefixed with menu- anymore. Hence the blocker and D8MI tags.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

The OP patch just applies this to some entities for demonstration to show how much duplicate code this helps remove. There is potential to remove LOTs more duplicate code removal.

Status: Needs review » Needs work

The last submitted patch, config-entity-links.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs followup
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -188,4 +195,48 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
+  public function uri() {
+    $entity_info = $this->entityInfo();
+    $link_templates = isset($entity_info['links']) ? $entity_info['links'] : array();
+
+    if (isset($link_templates['edit-form'])) {
+      $template = $link_templates['edit-form'];
+      $replacements = $this->uriPlaceholderReplacements();
+      $uri['path'] = str_replace(array_keys($replacements), array_values($replacements), $template);
+
+      // 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;
+      return $uri;
+    }
+
+    return parent::uri();
+  }
+
+  /**
+   * Returns an array of placeholders for this entity.
+   *
+   * Individual entity classes may override this method to add additional
+   * placeholders if desired. If so, they should be sure to replicate the
+   * property caching logic.
+   *
+   * @return array
+   *   An array of URI placeholders.
+   */
+  protected function uriPlaceholderReplacements() {
+    if (empty($this->uriPlaceholderReplacements)) {
+      $this->uriPlaceholderReplacements = array(
+        '{entityType}' => $this->entityType(),
+        '{id}' => $this->id(),
+        '{uuid}' => $this->uuid(),
+        '{' . $this->entityType() . '}' => $this->id(),
+      );
+    }
+    return $this->uriPlaceholderReplacements;

This looks like duplicate of EntityNG code, can we just move it to Entity? ConfigEntity doesn't have bundles, but EntityNG can override uriPlaceholderReplacements() and add in the bundle.

Also, I'd like to see a follow-up to move this to route_name instead of paths.

Gábor Hojtsy’s picture

FileSize
11.74 KB

All right. Copy-pasted down the uri() method from EntityNG then down to Entity. I did not change (and do not intend to change) anything in this code, it worked before. I want to make it apply to config entities. If the links array was not found and $rel == 'canonical', that is where the original Entity implementation ended up in. I made the bundle replacement logic happen in the base Entity, because that already has a concept of a bundle (same as entity type by default), so config entities will experience no harm.

I think there are an overwhelming number of reasons to unify the links system. It may be better to have them as routes, but they are not routes with content entities either and I am merely doing this to help minimise the repetition elsewhere. I am looking to have these URL patterns accessible when an entity type is known which is currently not possible, so it needs to be repeated in the code in #1952394: Add configuration translation user interface module in core all over again for each config entity. That is bad. I assume once/if these are converted to routes, then we would need to resolve those routes to paths in uri() and elsewhere where we need a path. That sounds like way out of scope for this indeed.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -188,4 +188,11 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
+  public function uri() {
+    return parent::uri('edit-form');
+  }

Nice!

I'm happy with this. Needs more eyes though.

Gábor Hojtsy’s picture

FileSize
11.75 KB

Patch did not apply anymore due to #2078341: Restructure Block UI and custom block pages. Here is one that applies.

Gábor Hojtsy’s picture

@tim.plunkett: thanks! It definitely needs all config entities converted. That will have a very impressive diffstat :)

Status: Needs review » Needs work

The last submitted patch, config-entity-links-5.patch, failed testing.

dawehner’s picture

Sorry for this kind of OT comment: On the longrun it would be great to not rely on hardcoded patterns in the entity definitions but rather use just route names.
Once you have done this, generated the URIs can be directly moved to the url generator and overriding an existing path can be done by just replace the route definition.

Gábor Hojtsy’s picture

@dawehner: once again I think this is orthogonal to this issue. This issue is about *providing the same declarative mechanism for edit forms for config entities like for content entities*. If/once the content entity method changes, this would need to change. This issue is about providing this information declaratively, and instead of making up something different, it is also about applying the same pattern then.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.71 KB
4.96 KB

Tests were failing because the uri_callback is now only invoked for config entities if for the "canonical" case, but we only ever ask for the edit form URL for the config entity (which ALL config entities actually used their uri_callbacks where defined). This patch brings in those URI callbacks to annotations links list instead for a unified implementation. I think this will pass now, it fixes all uses of uri_callback for config entities.

Status: Needs review » Needs work

The last submitted patch, config-entity-links-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
793 bytes
17.49 KB

The base/common URI method removes a trailing slash, so the config test needs to be updated. This was the only fail. Pretty simple :)

Gábor Hojtsy’s picture

Added the further missing items:

- entity form modes
- entity display modes
- node types
- picture mappings
- shortcut sets
- actions
- menus
- date formats
- vocabularies

I think this covers all config entities which can be covered. Field instances have a URI method, but they do need one on their own since they derive their URI from the parent entity. So not possible to declaratively define.

The diffstat for the patch is 25 files changed, 131 insertions(+), 288 deletions(-). Impressive, eh? 150 lines less code, applying the same pattern!

andypost’s picture

Status: Needs review » Needs work
Issue tags: -entity API, -Configuration system, -D8MI, -sprint, -language-config, -blocker, -Needs followup

The last submitted patch, config-entity-links-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +entity API, +Configuration system, +D8MI, +sprint, +language-config, +blocker, +Needs followup

#14: config-entity-links-14.patch queued for re-testing.

YesCT’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -149,36 +149,82 @@ public function label($langcode = NULL) {
    +    $link_templates = isset($entity_info['links']) ? $entity_info['links'] : array();
    +
    +    if (isset($link_templates[$rel])) {
    +      $template = $link_templates[$rel];
    +      $replacements = $this->uriPlaceholderReplacements();
    

    I was confused at first about these two ifs that return $uri, and what a link template was.

    Looks like for some entities, it gets the info/definition from the annotation, and if the links there includes the relationship being asked for (this patch sets edit-form in a bunch of cases) then it uses that pattern from the annotation.

    Otherwise if the type is not set in the info, and it is canonical, this tries a callback if one is set, and if not, sets a default link. I wonder if some comments in the overall doc block would be good, or more in that first section of code... but it is copied from what was already there so, maybe it is ok.

  2. I am not sure what happens (if anything should) if this gets called with a relationship type that is not canonical and is not set in the info.
  3. Also, can a canonical link template be set in annotations after this patch? (not just with the uri callback) Looks like it could be.
  4. uri() has a parameter now different from the interface definition, so I put the doc block with the @param in
  5. when some of the code was cut and pasted, it nested further so a comment went over 80 chars. I rewrapped it.
  6. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -149,36 +149,82 @@ public function label($langcode = NULL) {
    +    // For a canonical link (that is, a link to self), use the default logic.
    +    // Other relationship types are not supported by this logic.
    +    if ($rel == 'canonical') {
    +      $bundle = $this->bundle();
    

    I know it said look up the stack for default logic before this code was moved. But I am not sure that "use the default logic" makes sense in this location. I think this *is* the default logic.

    Is it better to say:

        // Only use these defaults for a canonical link (that is, a link to self).
        // Other relationship types are not supported by this logic.

    ? I changed it to that.

I will update the issue summary.

YesCT’s picture

Issue summary: View changes

updated remaining tasks and tried to clarify solution

Gábor Hojtsy’s picture

Status: Needs review » Needs work

1. Right, link templates are the values in the links {} array in annotations. You may want to update the docs for the function to make things clearer.

2. I don't want to change the behaviour at all, just extend it to config entities. What happens before the patch, if the method is invoked for a relationship that does not have a link template and is not canonical, it falls back on 'entity/' . $this->entityType . '/' . $this->id(), before and after the patch equally. This is a dumb default, its a URL that will not work, but that is the before/after code, and it is not our task here to fix/modify :)

3. Yup, a canonical URL template will be taken from the annotation, no problem.

4. Not sure about this. We should keep @inheritdoc for it to recognize this is an implementation of a parent/interface, no? Not using @inheritdoc at all for an implementation of an interface is an issue IMHO, no?

5. Superb.

6. Good improvement.

So in short I only have an issue with your change in (4).

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
26.67 KB

2. ok.

4. yeah.

#1994890: Allow {@inheritdoc} and additional documentation is about using inheritdoc *and* augmenting it with differences (the additional @param).

I was looking in core for examples and having trouble finding examples... to decide if it should be

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -149,7 +149,14 @@ public function label($langcode = NULL) {
+   * Implements \Drupal\Core\Entity\EntityInterface::uri().
+   *
+   * Returns the URI elements of the entity.
+   *
+   * @param string $rel
+   *   The link relationship type.
+   *
+   * @return
+   *   An array containing the 'path' and 'options' keys used to build the URI
+   *   of the entity, and matching the signature of url().

https://drupal.org/node/1354#inheritdoc
is not very helpful in the case of what to do if it is _not_ *exactly* the same docs, but implies to just follow "regular" guidelines.

1. ... tried to add the docs with examples.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Ok, well, I think there is only some terminology confusion left.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -149,36 +149,109 @@ public function label($langcode = NULL) {
+   * A URI pattern, might be set in the links array in an annotation, for
+   * example:
...
+   * If looking for the canonical URI, and it was not set in the links array
+   * or in a uri_callback function, the path is set using the default pattern:
...
+    // The links array might contain URI patterns set in annotations.
+    $link_templates = isset($entity_info['links']) ? $entity_info['links'] : array();
+
+    if (isset($link_templates[$rel])) {
+      // If there is a pattern for the given relationship type, do the pattern
+      // replacement and use that as the path.

I'm not sure introducing "pattern" as alternate terminology for "template" is a good idea. I'd explain template in the phpdoc instead and use that consistently.

The pattern terminology causes problems when you write "pattern replacement", because what happens is patterns are replaced in the template :) IMHO we may call {node}, etc. the patterns (but I would avoid using this word altogether).

Gábor Hojtsy’s picture

Issue summary: View changes

link to comment that added them and explains why not fields done.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
26.68 KB

I've read this like 8 times... so template is starting to make sense to me... oh no! Let's get someone to read the docs and see if they help.

Status: Needs review » Needs work
Issue tags: -entity API, -Configuration system, -D8MI, -sprint, -language-config, -blocker, -Needs followup

The last submitted patch, config-entity-links-22.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +entity API, +Configuration system, +D8MI, +sprint, +language-config, +blocker, +Needs followup

#22: config-entity-links-22.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I think the updated docs look good. With tim.plunkett's approval of the approach above, this looks good to go :)

joachim’s picture

Issue tags: +Needs change record
tim.plunkett’s picture

Issue tags: -Needs change record
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -149,36 +149,109 @@ public function label($langcode = NULL) {
+   * @return

This should be @return array

But that could be fixed on commit.

+1

Gábor Hojtsy’s picture

FileSize
26.67 KB

Manually edited in @return array :)

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -149,36 +149,109 @@ public function label($langcode = NULL) {
+  public function uri($rel = 'canonical') {

I think the additional argument here was a hack when it didn't match the interface and was in EntityNG. Now that the base class implements it, shouldn't we move the documentation and that argument to EntityInterface?

Gábor Hojtsy’s picture

@Berdir: ConfigEntityBase does not have it and does not support any variance, so it seems like it would still be correct to keep the interface as-is IMHO.

It would be great to see this in sooner than later since this helps with so much of DX of #1952394: Add configuration translation user interface module in core. Any comitters? :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +entity API, +Configuration system, +D8MI, +sprint, +language-config, +blocker, +Needs followup

The last submitted patch, config-entity-links-22.patch, failed testing.

Gábor Hojtsy’s picture

alexpott’s picture

Needs a reroll

Berdir’s picture

"@Berdir: ConfigEntityBase does not have it and does not support any variance"

I'm not sure if it shouldn't support any variance, shouldn't it just have a different default value? There is no reason to not support e.g. add/delete links too? e.g., add
// The canonical link of config entities is the edit form.
if $rel == canonical) $rel = 'edit-form'
in there.

(follow-up material:) I think we should really use edit-form and possibly delete-form explicitly in EntityListController. That might result in some fails for some entity types but then we can replace the blind /edit assumption there (and remove the just as blind override in config list controller that removes it again) and various overrides where it doesn't match those assumptions.

Gábor Hojtsy’s picture

@Berdir: right, extending the use of links to other things would modify the interface as well then I guess. This issue is about getting rid of the url() methods that only contain copy-paste stuff. It also unblocks DX issues elsewhere. I think it would indeed be great to expand on this in followups. I think the extent of change in this issue in itself is enough to move forward and then further steps can be made later on.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
26.65 KB

just context lines change from #2087995: All entity post*() and pre*() methods should call their parent implementation

rerolled.

same diff stats:
25 files changed, 158 insertions, 288 deletions.

YesCT’s picture

replacing some tags lost.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as it was for the past 6 days. :) The diffstat still looks amazing. Lots of duplicate code removed in favour of more standard declarative data.

alexpott’s picture

Title: Use links annotation for config entity URI like for content entities » Change notification: Use links annotation for config entity URI like for content entities
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 24e3c5d and pushed to 8.x. Thanks!

Let's get to work on the followups :)

Gábor Hojtsy’s picture

Title: Change notification: Use links annotation for config entity URI like for content entities » Use links annotation for config entity URI like for content entities
Priority: Major » Minor
Status: Active » Fixed
Issue tags: -Needs change record, -sprint

Change notice at https://drupal.org/node/2092261, I think this will be fine :)

Gábor Hojtsy’s picture

Priority: Minor » Normal
Status: Fixed » Reviewed & tested by the community
FileSize
932 bytes

Somehow blocks were missing from the original patch. Fortunately this was proven by the config translation integration tests that I'm writing so quickly caught :) Yay for more duplicate code removed. Going right to RTBC since this is a trivial followup.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d200436 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Perfect, thanks!

clemens.tolboom’s picture

Status: Needs work » Fixed

This change affected Tour UI has it's function tour_ui_uri() was not called anymore.

Somehow the Tour UI operations Edit and Delete where removed too.

So is the Change notice https://drupal.org/node/2092261 documented good enough?

Should we remove hook_uri() ?

tim.plunkett’s picture

Hmm, since all of the config entities had their uri() method overridden and passing 'edit-form', it means that if your config entity doesn't have an 'edit-form', but you were using uri_callback, it will not be used.

Meaning, the entire point of uri_callback, to override uri(), is broken for all config entities.

clemens.tolboom’s picture

I'm not sure they all added 'edit-form'.

$entity_info['tour']['links'] had only it's 'canonical' part. $entity_info['tour']['links']['edit-form'] was not set.

function tour_ui_entity_info_alter(&$entity_info) {
  $entity_info['tour']['links']['edit-form'] = 'admin/config/user-interface/tour/manage/{id}';
}

fixed it for Tour UI.

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
1.41 KB
2.52 KB

Maybe something like this? (no-interdiff version included for easier review)

Gábor Hojtsy’s picture

What is the use case for the uri_callback vs. the uri() method?

tim.plunkett’s picture

FileSize
5.06 KB
2.54 KB

Only the module that defines the entity can change uri().
This example was for a contrib tour_ui module.

Actually, re-reading the patch committed, the Menu entity type is wrong.
The system.module provides the Menu entity type, the menu.module provides the UI.

tim.plunkett’s picture

Argh, View is also wrong, that belongs in views_ui_entity_info() as well... Leaving for someone else, or tomorrow.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -198,36 +198,38 @@ public function uri($rel = 'canonical') {
+    // Invoke the callback to get the URI. If there is no callback, use the
+    // default URI format.
+    if (isset($uri_callback) && function_exists($uri_callback)) {
+      $uri = $uri_callback($this);
+    }

So now uri callbacks would be invoked for all kinds of $rel values, but they don't know which one they were invoked with. Is that the intention?

Also I think the docblock on this method should explain how uri callbacks are considered when resolving the path.

Schnitzel’s picture

Status: Fixed » Needs review
FileSize
5.62 KB

As discussed with Gabor, here the patch with a change to the docblock.

Schnitzel’s picture

Schnitzel’s picture

FileSize
5.63 KB

too tired to write proper english ;)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Amazing, let's get this in!

Status: Reviewed & tested by the community » Needs work
Issue tags: -DX (Developer Experience), -entity API, -d8dx, -Configuration system, -D8MI, -language-config, -blocker, -Needs followup

The last submitted patch, links-2083615-55.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +entity API, +d8dx, +Configuration system, +D8MI, +language-config, +blocker, +Needs followup

#55: links-2083615-55.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

added another related issue

dawehner’s picture

#1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views uses the url generator instead of str_replace based approach.