More specific titles for term and vocabulary edit forms

Modified Test.

Resolved the conflict happening for the forum & container add page.

Removed unwanted space.

Thanks for the patch! This change would be a good improvement for usability.

This change would break strings, so we cannot make it any more during RC. We could improve this in a minor version, though. Reference: https://www.drupal.org/core/d8-allowed-changes

+++ b/core/modules/taxonomy/src/TermForm.php
@@ -20,6 +20,10 @@ class TermForm extends ContentEntityForm {
+      $form['#title'] = $this->t('Edit ' . $term->label());

+++ b/core/modules/taxonomy/src/VocabularyForm.php
@@ -54,7 +54,7 @@ public function form(array $form, FormStateInterface $form_state) {
+      $form['#title'] = $this->t('Edit ' . $vocabulary->label());

These are not correct uses of t(). Variables should always be added to the string using string placeholders for security and to allow them to be translated properly with context. See the documentation of the method for details.

Edit: Note that I've postponed the issue for now since 8.1.x is not open for development yet. Once it is, we can create an updated patch that improves the use of t().

Patch updated with correct usage of t().

I think this is very much needed but I would suggest to also include the vocabulary name since they are very closely tied together and you have to get the context of which vocabulary that term belongs to. Like so:

Edit term "Apple" in "Fruits" vocabulary

See the related issue for dealing with the breadcrumbs #1905370: Add missing vocabulary to fix term edit form breadcrumbs

On the recommendation of #23 i've added : Edit term "Apple" in "Fruits" vocabulary

+++ b/core/modules/taxonomy/src/TermForm.php
@@ -15,6 +15,11 @@ class TermForm extends ContentEntityForm {
+      $form['#title'] = $this->t('Edit @name in @parent vocabulary', array('@name' =>  $term->label(),'@parent' => $term->bundle()));

I think since we're stating the type for the vocabulary name, we should also do this for the term: e.g. Edit term XXXX in YYYY vocabulary. And to mark the names a little better I would suggest to wrap the part of the text that is about the operation in s like we do that for the Node form.

So we would have:
Edit term XXXX in YYYY vocabulary
<em>Edit term</em> XXXX <em> in YYYY vocabulary</em>

+++ b/core/modules/taxonomy/src/TermForm.php
@@ -15,6 +15,11 @@ class TermForm extends ContentEntityForm {
+      $form['#title'] = $this->t('Edit @name in @parent vocabulary', array('@name' =>  $term->label(),'@parent' => $term->bundle()));

$term->bundle() will not give you the name of the vocabulary, but the machine name

I will provide a new patch for that

Here's an updated patch, I addressed the issues mentioned in #26 and added tests

Forgot to set the status ...

@pixelmord Thanks! For your patch,It works for me same as #26.

Fixed the last failing test

Problem with test is randomization of name for Forum, that makes order of forums different randomly -> once "General discussion" is first in list, once new created forum is in front. So I made small quick solution, that first letter starts with 'f' (forum) and then it will always be in front of "General discussion".

Let's see will that work on TestBot.

Ok, lets go back to RTBC

Patch, should still apply. Lets test

This is still green. Retested #34

Can we get before/after screenshots here? Also specifically of the Forum taxonomy forms changed as a consequence of the patch. The test changes needed to make Forum work with this patch are kinda suspicious and make me wonder if we need to add a different actual fix for Forum.

Even if we do use the current change for forum, we should probably not use the random name at all and just use an intentional test value.

I have changed testing a bit. Instead of changing random name, I have changed weight, so that new created forum is first in list. Changing randomization of name is another topic and it's not related with this issue. I personally prefer fixed names (so that tests do not flip-flop), but other developers prefer that tests discover problems.

I have attached screenshots. I have added for term and vocabulary in summary and here are forum related ones.

Before patch Edit of Forum:

After patch Edit of Forum:

Verified the patch '2596207_41.patch' in #41 on Drupal 8.4.x.The observations are as follows:
1.The patch got applied without any errors
2.The title for Edit Vocabulary is changed to 'Edit Vocabulary-name' eg . Edit Category
3.The title for Edit Vocabulary term is changed to Edit name in Vocabularyname vocabulary' eg. Edit test in Category vocabulary'
4.The title for Edit Forum is changed to 'Edit name in Forums vocabulary' eg 'Edit test in Forums vocabulary'
Attaching snapshot for reference
This can be marked as RTBC.

Re-roll.

Correct coding standards issues - "Short array syntax must be used to define arrays"

Linking to a similar issue, but for adding terms rather than editing them.

Hello,

I have applied the patch mentioned in comment #47 (2596207-47.patch). It is working as expected. PFA screenshots.

Adding

I feel with adding the vocabulary name to the title when editing a term we might be overdoing it a bit. I don't think we should use the title to express hierarchy or relationship.

Is it at all possible to show the vocabulary name in the breadcrumb instead?

Because that is what happens when creating a new term:

Needs a reroll.

Re-roll.

This isn't quite right.

Looking at nodes, the content entity, the node, has:

> /Edit Article/ Title

(i.e., all EM except the entity label)

The config entity, the node type, though has:

> Edit /Article/ content type

(i.e., only the entity label is in italic.)

Is that correct? That seems odd to me, but maybe there is a UX reason.

So either:

1. we follow suit, and the vocab edit page shoud be like the content type edit page
2. we file / look for an issue to see whether the italic discrepancy is indeed a discrepancy

The patch in #55 needs a reroll, as it causes a whitepage on systems with Drupal 8.7+. The Vocabulary class was moved (or removed), so Vocabulary::load fails. Possible replacement: $vocabulary = $term->vid->entity;

> The Vocabulary class was moved (or removed), so

I don't think that's the case:

8.7:

src/Entity/Vocabulary.php

8.8:

src/Entity/Vocabulary.php

Entity::load() still exists, so I don't know why that would cause a problem.

At any rate though, Entity::load() is not recommended, so $term->vid->entity is a good replacement just stylistically.

The Vocabulary class was moved (or removed)

That's indeed incorrect, my bad. I suspect our issue was actually that the patch didn't apply cleanly (possibly due to other patches), so we were missing the added use Drupal\taxonomy\Entity\Vocabulary; statement which in turn caused a whitepage:

Error: Class 'Drupal\taxonomy\Vocabulary' not found in Drupal\taxonomy\TermForm->form() (Zeile 23 in /srv/www/htdocs/my-site.url/html/core/modules/taxonomy/src/TermForm.php)

I've ported the patch for moved test files. I've also changed the title text slightly, to reduce the amount of emphasized text to <em>Edit</em> @name in %parent vocabulary (old: <em>Edit</em> @name <em>in @parent vocabulary</em>).
I didn't get the overridden titles on term forms, though that might be because of other modules. Let's see how the tests fare.

> I didn't get the overridden titles on term forms, though that might be because of other modules. Let's see how the tests fare.

Well, guess it wasn't just my setup then. But I wonder, why does the $form['#title'] not stick? The route's title would be "Edit term", but what's displayed is simply "My term name".

I edited the patch 62 to perform the following:
1 - When adding a new term (Add term) => Add "vocabulary_name".
2 - When Editing term (Edit term)=> Edit (term_name).
3 - When Editing vocabulary (Edit vocabulary) => Edit (vocabulary_name).

@qusai taha I think you missed some tests changes as per #62, I have added those and fixed fail tests, Please have a look.

Appiled patch #69 working fine now title added when edit vocublary for ref sharing screenshot ...

I've rerolled the patch from #70 against 9.3.x and added a test for the title change when adding taxonomy terms.

Verified and tested patch #69 on Drupal 9.5.x-dev. Patch applied successfully and looks good to me.
Testing steps:
1. Install 9.5
2. Visit /admin/structure/taxonomy/add
3. Add a new vocabulary.
4. Edit vocabulary.
5. Observe the "Edit Vocabulary" heading.
6. Apply patch #69
7. Observe the heading "Edit "
Testing Result:
1. After applying the patch heading of the edit page has changed. Screenshots are attached for reference:
Can be move to RTBC

Thanks everyone for working on this issue. Remember to read the comments before rerolling a patch. Comment #57 has not been addressed. Setting NW for discussing and consider #57.

The screenshots in the Issue Summary are to be from the latest patch. The Issue Summary also has states that this is not making User Interface changes and it is. That needs to be corrected. The Issue Summary needs to be updated. I am adding the tag for that.

I also don't see a code review here. That needs to be done as well.

@yashingole, thanks for listing what you did to test the patch.

Having a working patch is not usually sufficient to mark an issue RTBC. There are several steps, or Review a patch or merge request task of the Contributor guide is sufficient. The complete list of core gates has more topics. Also, check the tags on the issue and make sure they are complete.

Closed #3355627: Improve the Vocabulary edit form title as a duplicate

Please review.

Still needs an issue summary update

And not sure why this needs to change the forum test? Want to make sure we aren't just editing the tests to pass.

Hi @smustgrave. Forum module is linked with Taxonomy as the Forum container are getting added in taxonomy term and uses term and vocabulary forms. So the test of Forum module checks the title of vocabulary and term forms. Hope it is clear to you now. Thanks!

For the Vocabulary edit form title to apply this patch.

patch #90 is not valid

https://git.drupalcode.org/project/drupal/-/merge_requests/3883.diff is applying and working.

Summary updated.

If 3883 is the MR to be used it should be updated for 11.x and all patches hidden for clarity. Not familiar enough with the issue to make that call.

I have updated the MR for 11.x. Please have a look.

Left some feedback.

Updated the test for the new term addition as per comment #98.

Changing status to Needs Review as the test case is added and to get the clarity.

Addressed comment, also seems to have some test failures.

Thanks @smustgrave, added typehint for return type.