Deprecate TaxonomyController::vocabularyTitle()

Thanks, 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.

Added a draft change record.

@yo30: You need to remove the parentheses ("()") at the end of:

    $route->setDefault('_title_callback', 'Drupal\Core\Entity\Controller\EntityController::title()');

It should be:

    $route->setDefault('_title_callback', 'Drupal\Core\Entity\Controller\EntityController::title');

instead. I think the patch will be green then.

No worries, thanks for the updated patch. Let's see what the bot says.

Ahh, 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?

Thanks @yo30!! Unfortunately, I found one more minor problem:

+++ b/core/modules/taxonomy/src/Controller/TaxonomyController.php
@@ -34,8 +34,14 @@ public function addForm(VocabularyInterface $taxonomy_vocabulary) {
+   * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
+   *   Use Drupal\Core\Entity\Controller\EntityController instead.

This should be 8.5.0, as well, instead of 8.0.0.

Thanks, 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!

There are actually lots of valid reasons to have HTML in titles

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

Oh totally missed #15, sorry. That's certainly a "needs work", nice catch.

Thanks for picking this up!

I guess since this is now targeting 8.6 we also need to update all the deprecation notices unfortunately.

Perfect, thank you!

I 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?!

Ha, 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.