Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
With #2919891: Make Vocabulary use a route provider instead of hard-coded routes there is one usage of TaxonomyController::vocabularyTitle()
left. We should replace that usage with the generic EntityController::title()
. The only difference is that (weirdly) TaxonomyController::vocabularyTitle()
actually returns HTML, i.e. if the vocabulary label contains HTML, that is actually put as HTML into the DOM.
Tasks
- Replace
TaxonomyController::vocabularyTitle()
withEntityController::title()
inVocabularyRouteProvider::getOverviewPageRoute()
- Add a deprecation message to
TaxonomyController::vocabularyTitle()
pointing toEntityController::title()
(see https://www.drupal.org/core/deprecation) for more information
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff.txt | 1.41 KB | mohit1604 |
#28 | This_is_the_title___Site-Install_new.png | 10.01 KB | xjm |
#28 | This_is_the_title___Site-Install.png | 7.86 KB | xjm |
#25 | 2924408-25.patch | 2.18 KB | akoe |
#21 | 2924408-21.patch | 2.18 KB | hussainweb |
Comments
Comment #2
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedHere is my patch. I replaced with an un-deprecated function. Thanks.
Comment #3
tstoecklerThanks, that looks good for the first part of the issue.
Now we just need to add a
@deprecated
note to the function.I fixed the formatting in the issue summary now, I had messed that up before.
Comment #4
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedI added the @deprecated note to the function.
See my patch and review it. Thanks
Comment #5
chr.fritschDeprecations also needs a change record.
Comment #6
tstoecklerAdded a draft change record.
@yo30: You need to remove the parentheses ("()") at the end of:
It should be:
instead. I think the patch will be green then.
Comment #7
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedthanks, @tstoeckler , I was not a confirmed user, so no access to writing a change record.
I added the patch.
Comment #8
tstoecklerNo worries, thanks for the updated patch. Let's see what the bot says.
Comment #9
chr.fritschNot sure if we should add a trigger_error to the deprecated method
Comment #10
tstoecklerAhh, yes, you are right. So let's add a deprecation note as it is documented on https://www.drupal.org/core/deprecation
Are you up for that @yo30?
Comment #11
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedSure,
Thanks, @tstoeckler.
Comment #12
tstoecklerThanks @yo30!! Unfortunately, I found one more minor problem:
This should be 8.5.0, as well, instead of 8.0.0.
Comment #13
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedHere is the modification.:)
Comment #14
tstoecklerThanks, looks perfect to me now!
Just for the record @yo30. Next time when working on a core patch it's best practice to create an interdiff when uploading multiple consecutive patches. See https://www.drupal.org/documentation/git/interdiff for more information. In this case it was not a big deal, because the patch was so small, but it's nice to keep that in mind. Thanks!
Comment #15
xjmMakes sense, thanks.
We should also add an
@see
to the CR in the docblock (same as the link that's already in the message).Comment #16
xjmHmm, wait, so if the vocabulary title is
This <em>is</em> the title
, will this replacement result in the markup being escaped?There's an issue about markup in titles somewhere; we discussed it when we were working on the sanitization changes. There are actually lots of valid reasons to have HTML in titles. So let's make sure we're not regressing that.
Comment #17
xjmThere's #2563381: Hard to get page title and header both properly sanitized (very outdated) but that's not the one I was thinking of.
Comment #18
tstoecklerThat might be the case, but the fact of the matter is that entity labels are used in various contexts, not all of which are HTML. Or even if they end up being shown as part of the page, they might be already encapsulated in other HTML, as for example by
t('Edit %title')
. And by declaring the titles HTML we make all those use-cases either complicated or broken. Strictly speaking, what you are suggesting is that we treat the result of$entity->label()
as HTML instead of plain text and that would be a huge API change at this point. Furthermore, we don't do this for any entity types. So if there really is such a compelling reason to do this, then we should discuss in a broader, more general issue as I don't see any reason why this would be valid (and important) for vocabularies, but not for node types, comment types, etc. And even if we do open up a generic issue that's all the more reason for this issue to go in, so that we have more standardization and we can more easily change things across all entities.Attached are some before/after screenshots of the only visible changes of this patch: The page title on the term overview and the last breadcrumb component on the Manage fields/display/form display pages. As you can see both pages already show the escaped title, as well, making it clear that we are not in any way regressing, but in fact fixing an inconsistency here.
Going back to "Needs review" to hear your thoughts, but from my point of view this is still ready to fly.
Comment #20
tstoecklerOh totally missed #15, sorry. That's certainly a "needs work", nice catch.
Comment #21
hussainwebMade the changes as per #15. I also found few typos in the message (extra spaces) and I fixed that.
Demonstrated in Drupal Global Sprint Weekend 2018.
Comment #22
tstoecklerThanks for picking this up!
I guess since this is now targeting 8.6 we also need to update all the deprecation notices unfortunately.
Comment #23
akoe CreditAttribution: akoe commentedComment #24
akoe CreditAttribution: akoe commentedComment #25
akoe CreditAttribution: akoe commentedTested patch with core 8.6.x-dev and just updated core versions in comments.
Comment #26
akoe CreditAttribution: akoe commentedComment #27
tstoecklerPerfect, thank you!
Comment #28
xjmHm, I still kinda disagree; this is definitely a (small) regression. It looks like #2919891: Make Vocabulary use a route provider instead of hard-coded routes introduced a regression for the breadcrumb, and this is extending that regression to the title. So yes, it's adding consistency, but it's being consistently disruptive to introduce escaping where it was not there previously. :) It's not that allowing markup would be a new decision. We already allow and sanitize the markup in 8.4.x and earlier.
Before
After
At a minimum the CR needs to be a bit clearer about what the site builder should do instead, but I'm still not convinced we should do this.
Comment #29
tstoecklerI still don't understand why we have this feature for vocabularies if we don't have it anywhere else. Also not sure what specifically you would like to have in the change notice. We already note this change explicitly. What the site builders should do really depends on their use-case, but in all likelihood they should just remove the markup?!
Comment #30
larowlanFor #28
Comment #31
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedThanks for the patch, providing interdiff of patch #21 and #25 :)
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears to be a duplicate of #3037157: Vocabulary name not translated in page title of the taxonomy overview and reset forms
will move over credit.
Comment #42
tstoecklerHa, thanks for catching this @smustgrave!
For anyone seeing this, the linked issue did in fact do the same change that was discussed here and was considered problematic here. The issue of HTML was not brought up there, so either it was missed there or not deemed important.