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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

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.

Wim Leers’s picture

Title: quick edit of tags field leaves new tags unlinked » Quick 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.

joachim’s picture

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

joachim’s picture

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.

joachim’s picture

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.

Gábor Hojtsy’s picture

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.

Wim Leers’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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:

  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 :/).

Gábor Hojtsy’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

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

Update title, tags.

Wim Leers’s picture

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?

Gábor Hojtsy’s picture

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.

Wim Leers’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
8.45 KB
7.52 KB

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.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue tags: -sprint, -Spark
Wim Leers’s picture

Issue tags: +sprint, +Spark
Wim Leers’s picture

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.

Gábor Hojtsy’s picture

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 :)

webchick’s picture

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!

Wim Leers’s picture

Issue tags: -sprint

Thanks! :)

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

Anonymous’s picture

Issue summary: View changes

Updated steps to reproduce