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 Yes
Profiling (see comment #49) Yes

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

CommentFileSizeAuthor
#154 interdiff_150-154.txt1.03 KBravi.shankar
#154 2010132-154.patch7.2 KBravi.shankar
#150 interdiff_148-150.txt907 bytesravi.shankar
#150 2010132-150.patch7.2 KBravi.shankar
#148 2010132_148.patch7.19 KBaarti zikre
#147 reroll_diff_132-147.txt2.28 KBravi.shankar
#147 2010132-147.patch7.2 KBravi.shankar
#134 2010132-134.patch7.31 KByusufhm
#132 interdiff.2010132.130-132.txt3.57 KBlongwave
#132 2010132-132.patch7.15 KBlongwave
#130 interdiff_128_130.txt2.15 KBanmolgoyal74
#130 2010132-130.patch6.46 KBanmolgoyal74
#128 interdiff.2010132.127-128.txt813 byteslongwave
#128 2010132-128.patch6.46 KBlongwave
#127 interdiff.2010132.123-127.txt4.03 KBlongwave
#127 2010132-127.patch6.46 KBlongwave
#123 2010132-forum_uri-123.patch3.06 KBbrunodbo
#114 2010132-forum_uri-114.patch5.06 KBstBorchert
#105 profile_forum.route_processor_2.png17.8 KBflocondetoile
#105 profile_forum.route_processor.png22.97 KBflocondetoile
#104 interdiff-2010132-104.txt549 bytesflocondetoile
#104 2010132-104.patch5.11 KBflocondetoile
#102 interdiff.txt551 bytesDinesh18
#102 2010132-102.patch5.12 KBDinesh18
#98 2010132-forum-link.interdiff.txt1.16 KBpasan.gamage
#98 2010132-forum-link.98.patch5.12 KBpasan.gamage
#96 2010132-forum_uri_96-8.2.x.patch5.15 KBstBorchert
#95 2010132-forum_uri_95.patch5.16 KBstBorchert
#94 2010132-forum_uri_2-8.2.x.patch4.29 KBstBorchert
#85 interdiff-2010132.txt523 byteslongwave
#85 2010132-forum_uri.patch5.12 KBlongwave
#83 2010132-forum_uri.patch5.12 KBlongwave
#81 2010132-forum_uri.patch5.07 KBlongwave
#76 2010132-forum_uri.patch4.98 KBlongwave
#71 callgraph for forum RouteProcessor processOutbound.png299.8 KBevilehk
#70 convert-2010132-70.patch5.99 KBevilehk
#62 2010132-term-url-62.patch5.3 KBandypost
#57 2010132-term-url-57.patch5.34 KBandypost
#57 interdiff.txt1.5 KBandypost
#53 2010132-term-url-53.patch5.42 KBandypost
#51 2010132-term-url-51.patch5.05 KBdinarcon
#50 2010132-term-url-50.patch5.04 KBandypost
#45 2010132-term-url-44.patch5.07 KBandypost
#45 interdiff.txt375 bytesandypost
#42 2010132-term-url-42.patch5.03 KBandypost
#42 interdiff.txt1.82 KBandypost
#38 2010132-term-url-38.patch4.47 KBandypost
#38 interdiff.txt1.44 KBandypost
#38 2010132-term-url-38-fail.patch842 bytesandypost
#37 2010132-term-url-37.patch3.74 KBandypost
#37 interdiff.txt5.21 KBandypost
#34 2010132-34.patch3.54 KBandypost
#3 taxonomy-convert_taxonomy_term_uri-2010132-3.patch1.57 KBInternetDevels
#6 taxonomy-uri-2010132-6.patch1.51 KBdanylevskyi
#9 taxonomy-uri-2010132-9.patch1.47 KBdanylevskyi
#26 convert-2010132-26.patch1.03 KBevilehk
#30 interdiff-2010132-26-30.txt3.03 KBevilehk
#30 convert-2010132-30.patch3.85 KBevilehk
Support from Acquia helps fund testing for Drupal Acquia logo

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

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
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
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

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

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

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

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

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

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

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

re-roll

dinarcon’s picture

FileSize
5.05 KB

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

#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

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

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

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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

ytsurk’s picture

Using this as an approach to exchange the view of the taxonomy/term/% view for some vocabularies (or even terms).
Works flawlessly :D ! (Multilingual pathauto, field rendering etc.)

stBorchert’s picture

Re-roll for 8.2.x (if someone wants to use it with composer ;) ).

stBorchert’s picture

Re-roll for latest 8.3.x.
Added a check if the term can be loaded (which is necessary if the canonical url is requested for a non-existing term).

stBorchert’s picture

Same modifications for 8.2.x ...

The last submitted patch, 95: 2010132-forum_uri_95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pasan.gamage’s picture

larowlan’s picture

Issue tags: +Needs change record

I think we need a change record here too.

And we need some profiling.

Code is ready though.

flocondetoile’s picture

Just a little comment. Is there any reason to use the deprecated @entity.manager instead of @entity_type.manager into the service declaration ?

forum.route_processor:
  class: Drupal\forum\Routing\RouteProcessor
  tags:
    - { name: route_processor_outbound }
  arguments: ['@config.factory', '@entity.manager']
Berdir’s picture

Status: Needs review » Needs work

The reason is likely that the code was written before entity_type.manager actually existed and just has been rerolled/updated since then. In other words, no, lets update that.

Dinesh18’s picture

Status: Needs work » Needs review
FileSize
5.12 KB
551 bytes

Here is an updated patch and interdiff which implements #100

tacituseu’s picture

One spot left:

+   * @var \Drupal\Core\Entity\EntityManagerInterface
flocondetoile’s picture

I made some basic profiling with the timeline webprofiler module. The forum.route_processor took between 2-4ms in the full time line (Total time 739.8 ms Initialization time 24.8 ms).

forum.route_processor timeline
or
forum.route_processor timeline

larowlan’s picture

@flocondetoile how does that compare to HEAD?

flocondetoile’s picture

Re #106. I'm not sure I well understood your question. But I will try to answer anyway :-)

What I mean with the few tests done is that the rendering time of taxonomy term page takes between 2 and 4 ms more with the patch.
I profiled the full time line but this is not signifiant as often the rendering time for this page can easily vary from 50-100 ms between each tests. But in the detail of each timeline of the tests done, the time took specifically by the route processor introduced by this patch was always between 2 or 4 ms (for a total time of 750 ms for rendering the full page, of course with the cache enabled, so an impact of 0,0053% on the total time). Is it clearer ?

larowlan’s picture

Looks good, we need a change record now, then I think we'e done

flocondetoile’s picture

Issue summary: View changes

First draft of the change record : https://www.drupal.org/node/2897455

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

kerasai’s picture

Just re-ran #107 against 8.4.x, the patch still applies and all tests passed. Manual test confirms forum links are created as expected.

What else needs to be done to get this change record completed/reviewed and this issue RTBC'd?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

gnuget’s picture

I tested it against 8.5.x and there is one error but seems unrelated... :-/

stBorchert’s picture

Is there anything we could do to make this happen?

stBorchert’s picture

Meh, didn't meant to upload a new patch ... it does not differ from the one in 104.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

CR is filed, test added... looks good to go... not sure it needs profiling

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 114: 2010132-forum_uri-114.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

mlncn’s picture

Version: 8.6.x-dev » 8.9.x-dev
Status: Needs work » Reviewed & tested by the community

It appears that the patch in 114 is passing tests, contrary to testbot's old report?

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/forum/src/Routing/RouteProcessor.php
@@ -0,0 +1,58 @@
+        if ($term && $term->getVocabularyId() == $vid) {

getVocabularyId is deprecated - which is why the tests are failing, we need to use ::bundle instead

longwave’s picture

The problem now is that this is a new (even if very minor) deprecation so if this is targetted at 8.9.x we need a resolution of #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations before we can decide how to proceed.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

brunodbo’s picture

Status: Needs review » Needs work

The last submitted patch, 123: 2010132-forum_uri-123.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DrupalHack’s picture

Patch seems to resolve the issue, but results in breaking error on drush commands.

This is on 8.8.10

Symfony\Component\DependencyInjection\Exception\LogicException: Service 'forum.route_processor' for consumer 'route_processor_manager' does not implement Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface. in [error]
/app/docroot/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php:164
Stack trace:
#0 /app/docroot/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php(97): Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass->processServiceCollectorPass(Array, 'route_processor...',
Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#1 /app/vendor/symfony/dependency-injection/Compiler/Compiler.php(140): Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#2 /app/vendor/symfony/dependency-injection/ContainerBuilder.php(789): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#3 /app/docroot/core/lib/Drupal/Core/DrupalKernel.php(1335): Symfony\Component\DependencyInjection\ContainerBuilder->compile()
#4 /app/docroot/core/lib/Drupal/Core/DrupalKernel.php(916): Drupal\Core\DrupalKernel->compileContainer()
#5 /app/docroot/core/lib/Drupal/Core/DrupalKernel.php(840): Drupal\Core\DrupalKernel->initializeContainer()
#6 /app/docroot/core/includes/common.inc(1073): Drupal\Core\DrupalKernel->updateModules(Array, Array)
#7 /app/docroot/core/includes/utility.inc(55): drupal_flush_all_caches()
#8 /app/vendor/drush/drush/commands/core/cache.drush.inc(302): drupal_rebuild(Object(Composer\Autoload\ClassLoader), Object(Symfony\Component\HttpFoundation\Request))
#9 /app/vendor/drush/drush/includes/command.inc(422): drush_cache_rebuild()
#10 /app/vendor/drush/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#11 /app/vendor/drush/drush/includes/command.inc(199): drush_command()
#12 /app/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#13 /app/vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#14 /app/vendor/drush/drush/drush.php(12): drush_main()
#15 {main}

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

longwave’s picture

longwave’s picture

Really addressed #120 this time.

Status: Needs review » Needs work

The last submitted patch, 128: 2010132-128.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
6.46 KB
2.15 KB

->url() is deprecated and has been removed in 9.0.0. Used ->toUrl() instead.
https://www.drupal.org/node/2614344

I hope this is right. If yes, then we will have to update the CR as well.

Status: Needs review » Needs work

The last submitted patch, 130: 2010132-130.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

@anmolgoyal74 Thanks for spotting and fixing that.

Fixed test failure and coding standards issues.

andypost’s picture

It still needs profiling because outbound doing config read and that change more then forum pages

yusufhm’s picture

Version: 9.2.x-dev » 9.0.x-dev
FileSize
7.31 KB

I tried applying #132 to my current Drupal 9.0.8-based project and it was failing.

I've re-rolled the patch against the 9.0.x branch.

Status: Needs review » Needs work

The last submitted patch, 134: 2010132-134.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

yusufhm’s picture

Version: 9.0.x-dev » 9.2.x-dev
Status: Needs work » Needs review

Changing the version back to 9.2.x; I accidentally changed that one. I just wanted to have a patch that's compatible with 9.0.8.

cptX’s picture

Hi, I have applied patch #132 to drupal 9.1.2 in a multilingual site and I found an issue.

The manual url alias set in the forum page is not working in the other languages path. For example I get a path in english (base language):

/forum/international/batteries

but in greek I get

/gr/forum/54

but I should get /gr/forum/international/batteries

so it is not working in the other language path! I think this is a bug.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

hassebasse’s picture

Drupal 9.2.4, but also earlier versions

Patch 2010132-132.patch worked fine for the purpose it is aimed for, the Forum modules , but then I had a problem with the module background_image, that confused me as I have it installed on another site with no problems. The module gave error as soon as I tried to go into the settings of it.

After a lot of testings with reinstalling of backups, it turned out to be the above mentionnd patch that causes the problem, so I had to choose between background_image and the forum as I wanted it to work. Below I will add the errors I had on the background_image module:

Drupal\Core\Security\UntrustedCallbackException : Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was Drupal\background_image\Form\BackgroundImageForm::preRenderStates. See https://www.drupal.org/node/2966725 dans Drupal\Core\Render\Renderer->doTrustedCallback() (ligne 96 de /home/domaine.com/public_html/project/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php).

Error : Class "Drupal\forum\Routing\RouteProcessor" not found dans Drupal\Component\DependencyInjection\Container->createService() (/home/domain.com/public_html/project/web/core/lib/Drupal/Component/DependencyInjection/Container.php ligne 262)

My solution was to remove the above patch because if it can give problem to this module, I might do it to other modules as well.

leisurman’s picture

Patch 2010132-132 fixed our forum container and forum url paths on Drupal Version 9.2.6
I had to add this forum pathauto pattern
forums/[term:parent:name]/[term:name]
Then bulk delete and bulk re generate forum alias
Now the url paths look correct
http://site/container
http://site/container/forum
http://site/container/forum/topic
They used to look like this
http://site/123 (container)
http://site/124 (forum)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sir_squall’s picture

Hello,

Great work, I have just tested the patch "2010132-132" in 9.3.12 and everything is working well.

Thanks!

hassebasse’s picture

sir_squall,

be aware that it might cause problems for other modules, as I mention in post #139

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Dajka’s picture

Is it just me? After applying patch #132 i get the following error when clearing caches (drush cr)

In TaggedHandlersPass.php line 182:
                                                                                                                                          
  Service 'forum.route_processor' for consumer 'route_processor_manager' does not implement Drupal\Core\RouteProcessor\OutboundRouteProc  
  essorInterface.
sonam.chaturvedi’s picture

Status: Needs review » Needs work

Patch #132 does not apply successfully on 9.5.x-dev. Moving to Need Work.

$ git apply -v 2010132-132.patch
Checking patch core/modules/forum/forum.module...
Checking patch core/modules/forum/forum.services.yml...
Checking patch core/modules/forum/src/Routing/RouteProcessor.php...
Checking patch core/modules/forum/tests/src/Functional/ForumTest.php...
error: while searching for:

    // Test adding a comment to a forum topic.
    $node = $this->createForumTopic($this->forum, FALSE);
    $edit = [];
    $edit['comment_body[0][value]'] = $this->randomMachineName();
    $this->drupalPostForm('node/' . $node->id(), $edit, t('Save'));

error: patch failed: core/modules/forum/tests/src/Functional/ForumTest.php:236
error: core/modules/forum/tests/src/Functional/ForumTest.php: patch does not apply
Checking patch core/modules/forum/tests/src/Kernel/ForumLegacyTest.php...
ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
2.28 KB

Added reroll of patch #132 on Drupal 9.5.x.

aarti zikre’s picture

Resolve custom command fail

Status: Needs review » Needs work

The last submitted patch, 148: 2010132_148.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
907 bytes

Fixed failed tests of patch #148.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bronisMateusz’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Needs review » Reviewed & tested by the community

In my case, this patch worked:
https://www.drupal.org/project/drupal/issues/2010132#comment-13881399

with this pattern:
forums/[term:parent:name]/[term:name]

from comment:
https://www.drupal.org/project/drupal/issues/2010132#comment-14249165

longwave’s picture

Version: 9.5.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Needs work

As #151 states this change can only be committed to 10.1.x now.

  1. +++ b/core/modules/forum/forum.module
    @@ -115,22 +115,16 @@ function forum_entity_type_build(array &$entity_types) {
    + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
    + *   $forum->toUrl() instead.
    

    "is deprecated in drupal:10.1.0 and removed from drupal:11.0.0"

  2. +++ b/core/modules/forum/forum.module
    @@ -115,22 +115,16 @@ function forum_entity_type_build(array &$entity_types) {
    +  @trigger_error('forum_uri() is deprecated in Drupal 9.2.0 and is removed in Drupal 10.0.0. Use $forum->toUrl() instead. See https://www.drupal.org/node/2897455', E_USER_DEPRECATED);
    

    Same as above.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
1.03 KB

Addressed comment #153, please review.

Status: Needs review » Needs work

The last submitted patch, 154: 2010132-154.patch, failed testing. View results

quietone’s picture

Status: Needs work » Postponed

Forum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Jatin12nov’s picture

Hey @ravi.shankar The is not working with drupal 10.2
it patched the still facing the same issue with "forum/13" instead of "forum/all-discussion"
I see in the alias list, the alias is there for taxonomy but it redirects to "forum/13"
ANy solution to this?