Problem/Motivation

Currently rendering of canonical taxonomy term link for forum vocabulary is broken and have no test coverage.

To be more consistent with the object-oriented architecture need to implement Drupal\Core\Entity\EntityInterface::uri() method and drop taxonomy_term_uri() function and clean-up Term annotation uri_callback.

Proposed resolution

Implement a path processor outbound service to make this work again and add test. Remove taxonomy_term_uri() and forum_uri() functions as useless because entity templates are takes precedence.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
We have manually tested the patch and confirmed that it has not been rolled. Novice Instructions Yes
Reroll the patch. Instructions Yes
Draft a change record for the API changes Instructions
Profiling (see comment #49)

User interface changes

API changes

Original report by @andypost

Part of #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method

Needs to implement Plugin\Entity\Term.php uri() method and drop taxonomy_term_uri() function replacing it's calls to $term->uri() and clean-up Term annotation uri_callback

Comments

andypost’s picture

InternetDevels’s picture

Assigned:Unassigned» InternetDevels

We are working today with this issue during Code Sprint UA.

InternetDevels’s picture

Assigned:InternetDevels» Unassigned
Status:Active» Needs review
StatusFileSize
new1.57 KB
FAILED: [[SimpleTest]]: [MySQL] 55,854 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Patch attached.
All test runs succesfully at local machine.

podarok’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -132,6 +131,20 @@ class Term extends EntityNG implements TermInterface {
   }
+  ¶

https://drupal.org/coding-standards#indenting

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -132,6 +131,20 @@ class Term extends EntityNG implements TermInterface {
+  }
+  ¶

https://drupal.org/coding-standards#indenting

should be right indented due to drupal coding standarts

danylevskyi’s picture

Assigned:Unassigned» danylevskyi
danylevskyi’s picture

Assigned:danylevskyi» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,042 pass(es).
[ View ]
tstoeckler’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.php
@@ -134,6 +133,19 @@ public function id() {
+   * Implements Drupal\Core\Entity\EntityInterface::uri().

Should be {@inheritdoc} per the new standard.

Otherwise looks great. Let's get rid of these stinky old uri callbacks for good!

danylevskyi’s picture

Assigned:Unassigned» danylevskyi
danylevskyi’s picture

Assigned:danylevskyi» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,583 pass(es).
[ View ]
tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Yes, let's do this.

andypost’s picture

Status:Reviewed & tested by the community» Needs review

Not sure that right because we break alterability of the uri() (lets see what bot will show for forum tests

Discussion in #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method

InternetDevels’s picture

Status:Needs review» Needs work

taxonomy_term_uri callback rewrited in forum module to forum_uri.
So seems that this issue is blocked on #1970360: Entities should define URI templates and standard links Assigned to: linclark.
I have added this information to meta issue.

Also seems that this code will be better one

/**
   * {@inheritdoc}
   */
  public function uri(){
    $uri = parent::uri();
    $uri['path'] =  'taxonomy/term/' . $this->id();
    return $uri;
  }

I will also work globally with this at #1970360: Entities should define URI templates and standard links and #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method.
So we needs work here.

tstoeckler’s picture

Status:Needs work» Needs review

Can someone explain why this should be blocked on the forum issue specifically?
AFAICT forum should still be able to alter in the uri callback in hook_entity_bundle_info_alter() just like before. Just by default no uri callback is used.

andypost’s picture

@tstoeckler because once term gets overriden uri() method, so nothing is checking a term bundle ‘uri_callback’ override

tstoeckler’s picture

Status:Needs review» Needs work

You're totally right @andypost. I missed that. That's a super WTF, that the whole uri callback support is in uri() which can be overridden by any entity. All the more reason to get rid of this.

larowlan’s picture

Issue tags:+Needs tests

Yeah we have an issue here, because forum can't sub-class Term and hence inject its own uri for forum vocab terms.
It pains me to say it....but can we have an alter hook or something?
And if tests pass, that means we are missing test coverage for /forum urls (although I'm sure thats not the case).

tstoeckler’s picture

Can't we just change forum to use url-aliases instead of altering the uri?

OTOH there are probably a bunch of use-cases for uri-altering in contrib...
hmm

Berdir’s picture

Berdir’s picture

Issue summary:View changes

Updated issue summary.

andriyun’s picture

Issue summary:View changes
Issue tags:+dcdn-sprint
tvn’s picture

Issue tags:-dcdn-sprint
sreader’s picture

Issue summary:View changes
sreader’s picture

Issue summary:View changes
sreader’s picture

Issue summary:View changes
sreader’s picture

Issue summary:View changes
evilehk’s picture

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,850 pass(es).
[ View ]

Attached is a reroll from #9. The Term entities link definitions are now defined in an annotation, so the reroll is essentially removing the uri_callback in the annotion, and then removing the taxonomy_term_uri function.

evilehk’s picture

Issue summary:View changes

Hurray the tests passed! I've updated the issue summary to mark the reroll task as complete. There are no API changes, so I don't believe a change record is needed.

Swarnendu-Dutta’s picture

26: convert-2010132-26.patch queued for re-testing.

I submited for re-testing by mistake.

Berdir’s picture

While this is nice and now possible, the problem is that we are actually still using the uri_callback feature for forum terms, which override it for a specific bundle.

So we can move forward with this but it won't help us to remove the (by-bundle) uri_callback API in general.

evilehk’s picture

StatusFileSize
new3.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,864 pass(es).
[ View ]
new3.03 KB

Now and before the patch in #26, the forum_uri was only being called on the forums page. This was not always the case. With the URI templates defined in the links array set in Term annotations, the urlInfo method defined in Entity finds the link templates first and does not check for a uri_callback. As a result, forum_uri is not called and the taxonomy terms on a forum discussion have the url "taxonomy/term/{tid}" as opposed to "forum/{tid}".

This patch follows the advice on comment #17 and adds an alter. The forum module picks up this alter and changes the route name for terms in a forum vocabulary. Interdiff between #26 and #30 also included.

andypost’s picture

#30 looks great but exposes performance regression once a lot of terms should be rendered as links on page

Berdir’s picture

Status:Needs review» Needs work

Oh, we have no test coverage for that? We do need to add tests then, issue us already tagged as Needs Tests.

And if that's what we want to do ( a taxonomy term specific hook), then OK, but then we need to at least document that hook in taxonomy.api.php and make sure that the hook is properly namespaced, meaning, it should be hook_taxonomy_term_url_info() or so.

evilehk’s picture

Assigned:Unassigned» evilehk

I will add a test case, use proper namespacing, and update taxonomy.api.php.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience)
StatusFileSize
new3.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,585 pass(es), 441 fail(s), and 56 exception(s).
[ View ]

While there's no tests I think better to change precedence of link templates over URL provided by bundle settings.

Suppose "bundle settings first" makes more sense to allow contrib easy override core's entities "canonocals"

PS: I still sure that no reason to introduce a new hook, better to request API change and improve DX

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -153,11 +153,7 @@ public function urlInfo($rel = 'canonical') {
     // The links array might contain URI templates set in annotations.
     $link_templates = $this->linkTemplates();

this could be moved latter

Status:Needs review» Needs work

The last submitted patch, 34: 2010132-34.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.21 KB
new3.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,020 pass(es).
[ View ]

Reverted back a global Entity::urlInfo()

Also removed forum override by providing outbound processor.

The test case:
1) install forum
2) add node into "General discussion" forum
3) node view page should provide a link to forum/{node} via term field formatter

andypost’s picture

Issue tags:-Needs tests
StatusFileSize
new842 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,017 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.44 KB
new4.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,018 pass(es).
[ View ]

Added test

The last submitted patch, 38: 2010132-term-url-38-fail.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,33 @@
    +    if ($route->getPath() == '/taxonomy/term/{taxonomy_term}' && isset($parameters['taxonomy_term'])) {

    Hm, this isn't good, but routes don't know their own name, only the collection/provider know it. Maybe we should open an issue to pass the name to processOutbound (the calling code has it!) and add an @todo here

  2. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,33 @@
    +      if ($vid = \Drupal::config('forum.settings')->get('vocabulary')) {

    Can't this be injected?

evilehk’s picture

Assigned:evilehk» Unassigned
andypost’s picture

Parent issue:» #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method
StatusFileSize
new1.82 KB
new5.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,333 pass(es), 986 fail(s), and 1,131 exception(s).
[ View ]

Filed follow-up and @todo #2283851: Pass the route name to OutboundRouteProcessorInterface::processOutbound
Injected config factory

Also mention in #2010184-7: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method about removal of uri_callback annotation as well.

andypost’s picture

Category:Task» Bug report
Issue summary:View changes
tim.plunkett’s picture

+++ b/core/modules/forum/forum.services.yml
@@ -15,3 +15,7 @@ services:
+  forum.route_processor:
+    class: Drupal\forum\Access\RouteProcessor
+    tags:
+      - { name: route_processor_outbound }

+++ b/core/modules/forum/src/Access/RouteProcessor.php
@@ -0,0 +1,52 @@
+  function __construct(ConfigFactoryInterface $config_factory) {
+    $this->configFactory = $config_factory;
+  }

How is this being injected?

I hope this fails, or we're missing tests.

andypost’s picture

StatusFileSize
new375 bytes
new5.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,780 pass(es).
[ View ]

Sure

The last submitted patch, 42: 2010132-term-url-42.patch, failed testing.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

I think this is as good as it will get.

larowlan’s picture

+1 RTBC

catch’s picture

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs profiling

This looks like it could result in a performance regression, adding 'needs profiling' tag.

Instead of changing the URL when generating the term uris, it's now doing it for every uri. Would probably prefer the alter hook suggested in #17.

andypost’s picture

StatusFileSize
new5.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,213 pass(es).
[ View ]

re-roll

dinarcon’s picture

StatusFileSize
new5.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,916 pass(es).
[ View ]

Reroll

dawehner’s picture

  1. In general I wonder whether the getUrlInfo method should be overridable like the uri method used to be.
    +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,52 @@
    +      if ($vid = $this->configFactory->get('forum.settings')->get('vocabulary')) {

    Let's optimize it: Store the vocabulary forum setting in a member variable.

  2. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,52 @@
    +        if (Term::load($parameters['taxonomy_term'])->getVocabularyId() == $vid) {

    Let's use the entity manager instead.

andypost’s picture

StatusFileSize
new5.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es).
[ View ]

#52.1 there's nothing to optimize
2) fixed

jurcello’s picture

Status:Needs review» Needs work
Issue tags:+Amsterdam2014

I tested the patch and it works ok, but it seems to break the URL alias. To reproduce apply the patch, create a new forum and fill out the URL alias field. It will not work.

The actual alias record the the path module writes is still taxonomy/term/..
Debugging the path code using a debugger revealed that Drupal\Core\Url::getInternalPath uses a deprecated method, so maybe this problem is fixed when this deprecated method has been replaced by the proper method.

dawehner’s picture

  1. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,62 @@
    +   * @var \Drupal\Core\Config\ConfigFactoryInterface $configFactory

    The standard is a bit different here."@var \Drupal\Core\Config\ConfigFactoryInterface"

  2. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,62 @@
    +      if ($vid = $this->configFactory->get('forum.settings')->get('vocabulary')) {
    +        if ($this->entityManager->getStorage('taxonomy_term')->load($parameters['taxonomy_term'])->getVocabularyId() == $vid) {

    Given that we run this on potentially many taxonomy terms we should try to not call out to the config factory all the time but rather store this information in an easy accessible information

andypost’s picture

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
new5.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2010132-term-url-57.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fix #55.1 but 2 no idea... any reason to hold a state in service?

larowlan’s picture

  1. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,61 @@
    + * Contains \Drupal\forum\Access\RouteProcessor.

    is Access the right namespace here?

  2. +++ b/core/modules/forum/src/Access/RouteProcessor.php
    @@ -0,0 +1,61 @@
    +    $this->entityManager = $entity_manager;

    perhaps only store the storage controller instead of the entity manager? ie $this->termStorage

Other than that RTBC - and fixes #2350309: Forum index links head to taxonomy/term/{term} instead of forum/{term}

harijari’s picture

Issue tags:+dcwroc2014

dziabodo queued 57: 2010132-term-url-57.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 57: 2010132-term-url-57.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,589 pass(es).
[ View ]

re-roll
@larowlab #58 there's core/lib/Drupal/Core/Access/RouteProcessorCsrf.php and ForumBreadcrumbBuilderBase uses EM not storage

PS: still needs profiling according #49

dziabodo’s picture

Assigned:Unassigned» dziabodo
Status:Needs review» Active

Status:Active» Needs review

dziabodo queued 62: 2010132-term-url-62.patch for re-testing.

dziabodo queued 62: 2010132-term-url-62.patch for re-testing.

dziabodo’s picture

Assigned:dziabodo» Unassigned
YesCT’s picture

Issue summary:View changes
YesCT’s picture

Issue tags:-Novice

taking novice off since the novice task is done (for now)