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

Files: 
CommentFileSizeAuthor
#85 interdiff-2010132.txt523 byteslongwave
#85 2010132-forum_uri.patch5.12 KBlongwave
#71 callgraph for forum RouteProcessor processOutbound.png299.8 KBevilehk
#70 convert-2010132-70.patch5.99 KBevilehk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,503 pass(es). View
#62 2010132-term-url-62.patch5.3 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,589 pass(es). View
#57 2010132-term-url-57.patch5.34 KBandypost
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

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
FileSize
1.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
FileSize
1.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
FileSize
1.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.

andypost’s picture

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
FileSize
1.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

FileSize
3.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,864 pass(es). View
3.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)
FileSize
3.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
FileSize
5.21 KB
3.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
FileSize
842 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,017 pass(es), 1 fail(s), and 0 exception(s). View
1.44 KB
4.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
FileSize
1.82 KB
5.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

FileSize
375 bytes
5.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

FileSize
5.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,213 pass(es). View

re-roll

dinarcon’s picture

FileSize
5.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

FileSize
5.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
FileSize
1.5 KB
5.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}

jsobiecki’s picture

Issue tags: +dcwroc2014

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
FileSize
5.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’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)

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
evilehk’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,503 pass(es). View

Reroll of patch from comment #62.

evilehk’s picture

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

For profiling, I generated 1000 forum terms on the branch with the patch from comment #70. I loaded the forum page with xhprof enabled after a cache-rebuild. The page load was 6.5 seconds, with 9/10 of a second from Entity::url being called a thousand times. The route processor changes accounted for only 3.5/10 of those seconds The callgraph of the time in Drupal\forum\Access\RouteProcessor::processOutbound is attached. I'll run the same profile scenario against 8.0.x

dziabodo’s picture

Assigned: Unassigned » dziabodo
dziabodo’s picture

ok, the second approach to the topic, this time I'm much more motivated

mgifford’s picture

Assigned: dziabodo » Unassigned

mgifford queued 70: convert-2010132-70.patch for re-testing.

longwave’s picture

Title: Convert taxonomy_term_uri() to $term->uri() » Convert forum_uri() to $term->uri()
Component: taxonomy.module » forum.module
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
4.98 KB

Taking the liberty to move this to a major bug in forum.module, as this functionality simply doesn't work - the taxonomy term link on a forum node page takes you to the taxonomy listing, when it should take you to the forum listing instead.

As we need to retain backward compatibility I have removed the changes from taxonomy module and kept forum_uri() but marked it as deprecated.

andypost’s picture

taxonomy should be fixed as well
this is only override in core left, so do we still need uri overrides?
looks we should use link templates instead...

longwave’s picture

What is there to fix in taxonomy module? We cannot remove taxonomy_term_uri() now in case contrib or custom code is calling it, and we should recommend link templates instead but we cannot remove uri overrides again in case someone is already using them.

longwave’s picture

Related: in #2667040: Deprecate uri_callback in routes for entities I updated the docs in to strongly prefer link templates over uri callbacks.

Status: Needs review » Needs work

The last submitted patch, 76: 2010132-forum_uri.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
5.07 KB

Status: Needs review » Needs work

The last submitted patch, 81: 2010132-forum_uri.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
5.12 KB

Fixed the test, and also converted EntityManager to EntityTypeManager.

Berdir’s picture

I thought about adding per-bundle link template support, but see the discussion in #2645136: Clearly document the expected route name pattern for entities why we probably don't want to support that.

longwave’s picture

FileSize
5.12 KB
523 bytes

Spotted a typo in the comments. What else do we have to do to move this forward?

larowlan’s picture

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

Any reason this is in the Access namespace?

Apart from that this looks great - we could add a unit-test for the functionality too fwiw.

andypost’s picture

Suppose this needs profiling, the outbound processor is what we trying to aware til d7
I still not sure that it's right way

IMO this fine as it is but having decorator for link templates is better, we already using field overrides per bundle, so maybe a kind of that could be used for link template

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Title: Convert forum_uri() to $term->uri() » Canonical taxonomy term link for forum vocabulary is broken
xjm’s picture

Issue tags: -Entity Field API

Also removing the Entity Field API tag, since this bug seems pretty forum-specific. Thanks!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.