Problem/Motivation

To reproduce:

Create an article, and add some tags.
Then, quick edit the tags field and add a new tag.
Click away to the body field.
Click back to the tags field.
The newly added tag(s) are gone. BAM!

(The new tag is also not linked but that is by design, the same happens in preview on the regular node edit form).

Proposed resolution

Make those tags actually appear in the terms field. Render the new fields properly in the form item widget.

Remaining tasks

Commit.

User interface changes

Fixed new terms.

API changes

None.

Files: 
CommentFileSizeAuthor
#15 new-taxonomy-tid-gets-lost-edit-15-test-only.patch7.52 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,553 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#15 new-taxonomy-tid-gets-lost-edit-15.patch8.45 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,588 pass(es).
[ View ]
#15 interdiff.txt4.62 KBGábor Hojtsy
#13 new-taxonomy-tid-gets-lost-edit-13-tests-only.patch7.25 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 new-taxonomy-tid-gets-lost-edit-13.patch8.19 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,909 pass(es).
[ View ]
#9 new-taxonomy-tid-gets-lost-edit-9.patch960 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,936 pass(es).
[ View ]

Comments

That definitely did not happen before.

Doing a `git bisect` might help, but at the same time, that'll be … very slow to do, because a reinstall of Drupal will be necessary at every step. If you have the time to do this bisect work to figure out the commit that introduced this, that'd be super helpful and would expedite a solution for this.

Title:quick edit of tags field leaves new tags unlinkedQuick edit of taxonomy term field leaves new taxonomy terms unlinked
Status:Active» Closed (works as designed)

Oh, wait, a new term, you said! That's normal. That's because the term cannot be created until you hit the "Save" button in the toolbar, i.e. until you truly want to save things. Otherwise, we might create taxonomy terms that don't get used if you don't hit the "Save" button.

This is related to the whole #1510544: Allow to preview content in an actual live environment problem.

I didn't realize there was a save button in the toolbar!

Status:Closed (works as designed)» Active

Do you mean the pop-up toolbar? I did click that. The term is unlinked after I click Save in the pop-up toolbar and the node has gone back to normal.

Confirming that the new taxonomy term has been created and is listed in the admin section when I am back on te node with the unlinked term.

Well, so this is indeed happening. When the field renders back, the terms are not yet saved, so we don't get the final linked versions. When the editing exits, the rendered version is still the pre-save version and the save happens later on in a different HTTP request. There is no re-rendering of the items when that happens. If you reload the page, it will appear properly though. This is indeed a problem, but I'm not sure how to resolve this. We don't really know which fields do we need to go back to and re-render after the quick edit is finished. We can sort of reload the whole page, but that is not very nice either...

A bigger problem that seems to also happen but may not be related is that if you add new taxonomy terms, NOT save it, go into a separate field and then go back to the term field, you don't get those back. That seems to be an issue with the tempstore copy of the entity not getting back the terms properly. Which in turn maybe related to how they are saved if they don't have term ids. Duh.

A bigger problem that seems to also happen but may not be related is that if you add new taxonomy terms, NOT save it, go into a separate field and then go back to the term field, you don't get those back. That seems to be an issue with the tempstore copy of the entity not getting back the terms properly. Which in turn maybe related to how they are saved if they don't have term ids. Duh.

Yep, I've seen that too, and have reported it numerous times, but it must've gotten lost in the so many things that we're working on.

The only way I can see this working is if nonmaterialized referenced entities (because that's what a new taxonomy term is: an entity that's being referenced but isn't stored yet) are somehow also stored in TempStore and only when we hit "Save", they will actually hit the DB. But that means that the whole system needs a way of knowing that entities with certain special IDs live in TempStore
I think that might require pretty big API changes though :(

Note: all of this is also why #1510544: Allow to preview content in an actual live environment isn't making progress.

I think we may be able to reproduce the same "fake" data that the form has for the previewed terms to make them work.

StatusFileSize
new960 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,936 pass(es).
[ View ]

Ok, so this is just a variant of #1985138: New autocomplete taxonomy terms not previewable really. What happens in the regular taxonomy editing form is that Drupal\taxonomy\Plugin\field\widget\TaxonomyAutocompleteWidget's formElement() is invoked with the items. This does not put in a correct list of taxonomy terms in the default values list. This is however pretty much flies under the radar, because the autocomplete field also has a validation callback, taxonomy_autocomplete_validate(), which repopulates! the field values this time with the correct values from the form build (instead of the field). So this ensures that the value is actually correct. If not for the validation function, the yet non-existent terms would not show up. Why?

Well, the formElement() method has this code:

<?php
 
public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, array &$form_state) {
   
$tags = array();
    foreach (
$items as $item) {
     
$tags[$item->target_id] = isset($item->taxonomy_term) ? $item->taxonomy_term : entity_load('taxonomy_term', $item->target_id);
    }
  ...
?>

This is wrong on many levels. First, it keys the elements by target_id. For yet non-existent elements, the target-id is always 0. So even if the rest of the line would do something useful, only one of the newly added terms would show up, not all of them. It does not really matter what we key this array with, we just implode the values later on, so we should skip using target_id.

Then it attempts to use taxonomy_term as the loaded term. Reality is that this is not anymore the way the taxonomy code actually behaves. If you look at massageFormValues() in the same class, you'll see there is either a target_id which has the tid to load the entity with OR there is a target_id of 0 *and* an 'entity'(!) property with a full (fake) runtime entity for the yet unsaved entity. Not a taxonomy_term key at all. So once we go and look at that property instead, all is well :)

So why does this surface with edit only? Looks like the validation callback is for some reason not attached or invoked with edit. We could/should fix that as well, however, the updated code already does all the things needed without the validation function painting one more layer on top.

All in all the fix is very simple (and not very representative of the pain endured :/).

Status:Active» Needs review

Title:Quick edit of taxonomy term field leaves new taxonomy terms unlinkedQuick edit of taxonomy term field looses new terms if switching out/back to field
Issue tags:+sprint, +Spark

Update title, tags.

Component:edit.module» taxonomy.module

Holy cow, that is some epic debugging!

Does anything else need to happen before this can be RTBC'd? How feasible and useful is it to write test coverage for this?

StatusFileSize
new8.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,909 pass(es).
[ View ]
new7.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Tada, here are tests! Basically I do the steps to reproduce in the test. Create an autocomplete field for tags, save two tags, and try to use those two tags and a new tag in the field. WIthout the fix, the tag does not come back in the form, with the fix it does.

Note that in both cases *the test* saves the terms properly at the end. That is due to us not saving back the form after it comes back incomplete (which however edit module does). That would be more test code to demonstrate even more failures. I think since this demonstrates the fail in itself, it should be fine.

I thought it would be important to have full-on web tests for this, since we continually had autocomplete taxonomy term issues in the life of edit so far. So an all-around full circle webtest should prove both existing and new terms work fine, come back in the form, etc.

Status:Needs review» Needs work
  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    + * Tests edit autocomplete.

    Tests using in-place editing for an autocomplete entity reference widget.

  2. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +    $this->vocabulary = entity_create('taxonomy_vocabulary',  array(
    ...
    +    $this->field_name = 'field_' . $this->vocabulary->id();

    $this->vocabulary is not documented. Since it is also used in other methods of this test class, just making it a local variable won't do: documenting it is necessary.

    Same for $this->field_name.

  3. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +    $this->vocabulary = entity_create('taxonomy_vocabulary',  array(
    ...
    +    $this->field_name = 'field_' . $this->vocabulary->id();

    Same here.

  4. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], '>' . $this->term1->label() . '<'), 'Existing term 1 in rendered output.');
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], '>' . $this->term2->label() . '<'), 'Existing term 2 in rendered output.');

    It should be asserted that these two are links.

  5. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], 'new term'), 'New term in rendered output.');

    It should be asserted that this one is *not* a link.

  6. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +      // Load the form again, which should now get it back from tempstore.

    s/tempstore/TempStore/

  7. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +      // Response should still contain the tags.

    This is too vague, IMO. It should say something like "The AjaxResponse's first command is an InsertCommand which contains the form to edit the taxonomy term field, it should contain all three taxonomy terms, including the one that has just been newly created and which is not yet stored."

  8. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], $this->term1->label()), 'Existing term 1 in reloaded form.');
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], $this->term2->label()), 'Existing term 2 in reloaded form.');
    +      $this->assertTrue(strpos($ajax_commands[0]['data'], 'new term'), 'New term in reloaded form.');

    I think these assertions could be more strict, but it's acceptable.

  9. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +   * Returns a new term with random properties in vocabulary $vid.

    s/properties/name and description/

  10. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditAutocompleteTermTest.php
    @@ -0,0 +1,201 @@
    +   * @return \Drupal\taxonomy\Term

    Should be the interface.

Status:Needs work» Needs review
StatusFileSize
new4.62 KB
new8.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,588 pass(es).
[ View ]
new7.52 KB
FAILED: [[SimpleTest]]: [MySQL] 58,553 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

All right, made all those suggested changes. I used drupalSetContent() so we can use the regular assert methods which are nicer. I made the form assert for example to assert for the form item value by name in a very concrete way. Now this assumes a lot about the taxonomy field behaviour, which I was not sure to test (hence the less strict assertions above) but you wanted to be more strict. Not sure we can get any more stricter than this in a sensible way.

Status:Needs review» Needs work

The last submitted patch, new-taxonomy-tid-gets-lost-edit-15.patch, failed testing.

Status:Needs work» Needs review

The first patch in #15 should have 1 fail, not 2, and the second patch should have no fails, not 1. Both had 1 more fail than expected due to #2078813: Create hook_help for telephone module breaking HEAD yesterday.

Re-testing.

Issue tags:-sprint, -Spark

Issue tags:+sprint, +Spark

Assigned:Unassigned» Gábor Hojtsy
Status:Needs review» Reviewed & tested by the community
Issue tags:+quickfix

Alrighty, this is good to go now!

Since it's a one line fix, I think this qualifies as a "quickfix", even though we add a bunch of regression test coverage.

Right. Although it is a one line fix, we add lots of test coverage on top because taxonomy terms had multiple issues before and we need full integration testing for edit to ensure they work for once and for all :)

Status:Reviewed & tested by the community» Fixed

This is awesome!! Great find, and even better, test coverage!! :D

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Thanks! :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated steps to reproduce