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
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
Comment | File | Size | Author |
---|---|---|---|
#154 | interdiff_150-154.txt | 1.03 KB | ravi.shankar |
#154 | 2010132-154.patch | 7.2 KB | ravi.shankar |
#150 | interdiff_148-150.txt | 907 bytes | ravi.shankar |
#150 | 2010132-150.patch | 7.2 KB | ravi.shankar |
#148 | 2010132_148.patch | 7.19 KB | aarti zikre |
Comments
Comment #1
andypostrelated #2008616: Convert user_uri() to $user->uri()
Comment #2
InternetDevels CreditAttribution: InternetDevels commentedWe are working today with this issue during Code Sprint UA.
Comment #3
InternetDevels CreditAttribution: InternetDevels commentedPatch attached.
All test runs succesfully at local machine.
Comment #4
podarokhttps://drupal.org/coding-standards#indenting
https://drupal.org/coding-standards#indenting
should be right indented due to drupal coding standarts
Comment #5
danylevskyiComment #6
danylevskyiComment #7
tstoecklerShould be {@inheritdoc} per the new standard.
Otherwise looks great. Let's get rid of these stinky old uri callbacks for good!
Comment #8
danylevskyiComment #9
danylevskyiComment #10
tstoecklerYes, let's do this.
Comment #11
andypostNot 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
Comment #12
InternetDevels CreditAttribution: InternetDevels commentedtaxonomy_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
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.
Comment #13
tstoecklerCan 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.
Comment #14
andypost@tstoeckler because once term gets overriden uri() method, so nothing is checking a term bundle ‘uri_callback’ override
Comment #15
tstoecklerYou'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.
Comment #16
andypostRelated forum issue #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
Comment #17
larowlanYeah 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).
Comment #18
tstoecklerCan'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
Comment #19
BerdirSee #2010184-1: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method, last paragraph...
Comment #19.0
BerdirUpdated issue summary.
Comment #20
andriyun CreditAttribution: andriyun commentedComment #21
tvn CreditAttribution: tvn commentedComment #22
sreader CreditAttribution: sreader commentedComment #23
sreader CreditAttribution: sreader commentedComment #24
sreader CreditAttribution: sreader commentedComment #25
sreader CreditAttribution: sreader commentedComment #26
evilehk CreditAttribution: evilehk commentedAttached 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.
Comment #27
evilehk CreditAttribution: evilehk commentedHurray 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.
Comment #28
Swarnendu-Dutta CreditAttribution: Swarnendu-Dutta commented26: convert-2010132-26.patch queued for re-testing.
I submited for re-testing by mistake.
Comment #29
BerdirWhile 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.
Comment #30
evilehk CreditAttribution: evilehk commentedNow 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.
Comment #31
andypost#30 looks great but exposes performance regression once a lot of terms should be rendered as links on page
Comment #32
BerdirOh, 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.
Comment #33
evilehk CreditAttribution: evilehk commentedI will add a test case, use proper namespacing, and update taxonomy.api.php.
Comment #34
andypostWhile 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
Comment #35
andypostthis could be moved latter
Comment #37
andypostReverted 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 formatterComment #38
andypostAdded test
Comment #40
tim.plunkettHm, 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
Can't this be injected?
Comment #41
evilehk CreditAttribution: evilehk commentedComment #42
andypostFiled 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.Comment #43
andypostComment #44
tim.plunkettHow is this being injected?
I hope this fails, or we're missing tests.
Comment #45
andypostSure
Comment #47
tim.plunkettI think this is as good as it will get.
Comment #48
larowlan+1 RTBC
Comment #49
catchThis 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.
Comment #50
andypostre-roll
Comment #51
dinarcon CreditAttribution: dinarcon commentedReroll
Comment #52
dawehnerLet's optimize it: Store the vocabulary forum setting in a member variable.
Let's use the entity manager instead.
Comment #53
andypost#52.1 there's nothing to optimize
2) fixed
Comment #54
jurcello CreditAttribution: jurcello commentedI 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.
Comment #55
dawehnerThe standard is a bit different here."@var \Drupal\Core\Config\ConfigFactoryInterface"
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
Comment #56
andypostRoute name is fixed by #2346283: Add route name parameter to OutboundRouteProcessorInterface
Comment #57
andypostFix #55.1 but 2 no idea... any reason to hold a state in service?
Comment #58
larowlanis Access the right namespace here?
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}
Comment #59
jsobiecki CreditAttribution: jsobiecki commentedComment #62
andypostre-roll
@larowlab #58 there's
core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
andForumBreadcrumbBuilderBase
uses EM not storagePS: still needs profiling according #49
Comment #63
dziabodo CreditAttribution: dziabodo commentedComment #66
dziabodo CreditAttribution: dziabodo commentedComment #67
YesCT CreditAttribution: YesCT commentedComment #68
YesCT CreditAttribution: YesCT commentedtaking novice off since the novice task is done (for now)
Comment #69
jhedstromComment #70
evilehk CreditAttribution: evilehk commentedReroll of patch from comment #62.
Comment #71
evilehk CreditAttribution: evilehk commentedFor 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
Comment #72
dziabodo CreditAttribution: dziabodo commentedComment #73
dziabodo CreditAttribution: dziabodo commentedok, the second approach to the topic, this time I'm much more motivated
Comment #74
mgiffordComment #76
longwaveTaking 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.
Comment #77
andyposttaxonomy 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...
Comment #78
longwaveWhat 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.
Comment #79
longwaveRelated: in #2667040: Deprecate uri_callback in routes for entities I updated the docs in to strongly prefer link templates over uri callbacks.
Comment #81
longwaveComment #83
longwaveFixed the test, and also converted EntityManager to EntityTypeManager.
Comment #84
BerdirI 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.
Comment #85
longwaveSpotted a typo in the comments. What else do we have to do to move this forward?
Comment #86
larowlanAny reason this is in the Access namespace?
Apart from that this looks great - we could add a unit-test for the functionality too fwiw.
Comment #87
andypostSuppose 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
Comment #89
xjmComment #90
xjmAlso removing the Entity Field API tag, since this bug seems pretty forum-specific. Thanks!
Comment #93
ytsurkUsing 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.)
Comment #94
stBorchertRe-roll for 8.2.x (if someone wants to use it with composer ;) ).
Comment #95
stBorchertRe-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).
Comment #96
stBorchertSame modifications for 8.2.x ...
Comment #98
pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commentedfixes #86
Comment #99
larowlanI think we need a change record here too.
And we need some profiling.
Code is ready though.
Comment #100
flocondetoileJust a little comment. Is there any reason to use the deprecated @entity.manager instead of @entity_type.manager into the service declaration ?
Comment #101
BerdirThe 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.
Comment #102
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is an updated patch and interdiff which implements #100
Comment #103
tacituseu CreditAttribution: tacituseu commentedOne spot left:
Comment #104
flocondetoileupdated
Comment #105
flocondetoileI 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).
or
Comment #106
larowlan@flocondetoile how does that compare to HEAD?
Comment #107
flocondetoileRe #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 ?
Comment #108
larowlanLooks good, we need a change record now, then I think we'e done
Comment #109
flocondetoileFirst draft of the change record : https://www.drupal.org/node/2897455
Comment #111
kerasai CreditAttribution: kerasai at myDropWizard commentedJust 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?
Comment #113
gnugetI tested it against 8.5.x and there is one error but seems unrelated... :-/
Comment #114
stBorchertIs there anything we could do to make this happen?
Comment #115
stBorchertMeh, didn't meant to upload a new patch ... it does not differ from the one in 104.
Comment #116
andypostCR is filed, test added... looks good to go... not sure it needs profiling
Comment #119
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedIt appears that the patch in 114 is passing tests, contrary to testbot's old report?
Comment #120
larowlangetVocabularyId is deprecated - which is why the tests are failing, we need to use ::bundle instead
Comment #121
longwaveThe 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.
Comment #123
brunodboI came here from #2680503: Fatal error: Call to a member function get() on null in [mysite]/modules/pathauto/src/Plugin/pathauto/AliasType/ForumAliasType.php on line 45 and #2896731: pathauto not working for forum and container.
This is a reroll of the patch in #114, with the change mentioned in #120.
Comment #125
DrupalHack CreditAttribution: DrupalHack commentedPatch seems to resolve the issue, but results in breaking error on drush commands.
This is on 8.8.10
Comment #127
longwave#123 is missing a file from #114. Re-added that, addressed #120, updated the deprecation message now we are targetting 9.2.x and used the standard format, added a proper deprecation error and a deprecation test.
Comment #128
longwaveReally addressed #120 this time.
Comment #130
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented->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.
Comment #132
longwave@anmolgoyal74 Thanks for spotting and fixing that.
Fixed test failure and coding standards issues.
Comment #133
andypostIt still needs profiling because outbound doing config read and that change more then forum pages
Comment #134
yusufhmI 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.
Comment #136
yusufhmChanging 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.
Comment #137
cptX CreditAttribution: cptX commentedHi, 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.
Comment #139
hassebasse CreditAttribution: hassebasse commentedDrupal 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.
Comment #140
leisurman CreditAttribution: leisurman commentedPatch 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)
Comment #142
sir_squall CreditAttribution: sir_squall commentedHello,
Great work, I have just tested the patch "2010132-132" in 9.3.12 and everything is working well.
Thanks!
Comment #143
hassebasse CreditAttribution: hassebasse commentedsir_squall,
be aware that it might cause problems for other modules, as I mention in post #139
Comment #145
Dajka CreditAttribution: Dajka commentedIs it just me? After applying patch #132 i get the following error when clearing caches (drush cr)
Comment #146
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at Salsa Digital commentedPatch #132 does not apply successfully on 9.5.x-dev. Moving to Need Work.
Comment #147
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #132 on Drupal 9.5.x.
Comment #148
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedResolve custom command fail
Comment #150
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed tests of patch #148.
Comment #152
bronisMateusz CreditAttribution: bronisMateusz at Smartbees commentedIn 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
Comment #153
longwaveAs #151 states this change can only be committed to 10.1.x now.
"is deprecated in drupal:10.1.0 and removed from drupal:11.0.0"
Same as above.
Comment #154
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed comment #153, please review.
Comment #156
quietone CreditAttribution: quietone at PreviousNext commentedForum 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.
Comment #158
Jatin12nov CreditAttribution: Jatin12nov commentedHey @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?