Add entity type label for a collection of entities

We'll end up with a million labels :)

See also #2507235: EntityType::getLowercaseLabel() breaks translation

Patch looks good to me and makes sense given all the other labels we have in the annotation now. But yeah, I agree with Berdir in #5 too :/

This could have perhaps been a 'labels' array in the annotation, much like we have 'handlers', 'links', etc.

This could have perhaps been a 'labels' array in the annotation, much like we have 'handlers', 'links', etc.

This sounds like a good idea in general, I think. On the other hand this doesn't really belong here. We could have its own issue to discuss this change and introduce the needed BC layers.

Go on then :)

I guess this issue should really remove the @todo in DefaultHtmlRouteProvider::getCollectionRoute too.

Hopefully this can be done at commit to save any of use rolling a new patch.

moving back to RTBC

Given we didn't get a green test run, probably worth removing the @toddo and uploading the patch again.

Sometimes testbot seems to go crazy like that. It doesn't necessarily mean that it's this issue that broke it. I am curious as to the results of re-uploading the same patch, though :)

Good catch. Updated.

- // @todo Improve this in https://www.drupal.org/node/2767025
- '_title' => '@label entities',
- '_title_arguments' => ['@label' => $entity_type->getLabel()],
+ '_title' => (string) $entity_type->getCollectionLabel(),

I may be wrong but aren't route titles supposed to be untranslated? Because _title_arguments and _title_context is later on used to pass the whole thing to t() along with _title?

I think getLabel() would return a TranslatableMarkup, so we can get the source string / any placeholders and merge them in to the route parameters? Otherwise it does seem like @kristiaanvandeneynde has a valid concern.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -628,6 +628,14 @@ public function getLabel();

@@ -628,6 +628,14 @@ public function getLabel();
   public function getLowercaseLabel();
 
   /**
+   * Gets the collection label of the entity type.
+   *
+   * @return string
+   *   The collection label.
+   */
+  public function getCollectionLabel();
+

This interface is not marked internal. Are we allowed to change it (thus make anyone implementing it WSOD/fatal)?

Also tagging for workflow initative based on #2811407: "Workflow entities" admin page title should be "Workflows" and D8MI sprint.

One could argue that all entity types should extend EntityType which implements the methods from EntityTypeInterface therefore this is not a backwards compatibility break. However we've already had comments when using the same argument for ContentEntityBase and ContentEntityInterface.

Needs release manager review for the allowed changes.

I think only the changes suggested by @kristiaanvandeneynde would be outstanding otherwise.

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -756,6 +763,17 @@ public function getLowercaseLabel() {
+      $this->label_collection = new TranslatableMarkup('@label entities', ['@label' => $label], [], $this->getStringTranslation());

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
@@ -320,9 +320,7 @@ protected function getCollectionRoute(EntityTypeInterface $entity_type) {
-          '_title_arguments' => ['@label' => $entity_type->getLabel()],
+          '_title' => $entity_type->getCollectionLabel()->getUntranslatedString(),
         ])

We should also bring over title arguments from the method's return value as demonstrated by the default implementation, otherwise we'll end up with "@label entities" without the placeholder value :)

Looks good then. :) If a committer looks at this and believes release manager review is not needed they can still remove the tag and commit :)

Yeah, this looks like a sensible and normal API addition for a minor release, and 8.3.x is still open, so no release management review needed. Leaving RTBC since I haven't reviewed this in depth. Thanks!

Looks great!

Re #28: I found out about it when writing my own permission handler that core tends to work with untranslated strings for almost anything that can go into a YAML file. It's only translated once "loaded".

This is definitely vastly preferable to the amount of backflips and somersaults needed to do something as simple as #2811407: "Workflow entities" admin page title should be "Workflows" right now. I see @Berdir is a bit lukewarm to the idea, but it seems like a minimally invasive approach for now, which doesn't make the existing situation much worse, and allows us to explore something along the lines of #9 for 8.4.x.

Committed and pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Let's get a change record for this as well, please.

Thanks @tstoeckler!

I opened #2869039: Group all entity labels under a single 'labels' annotation key for implementing the suggestion in #7.