Problem

1.
- Create a node with no tags.
- Attempt to add a tag and preview. (Experience your term showing up in preview with a link to "/taxonomy/term" - note no tid, this is expected)

2.
- Save the node, now it has a tag.
- Go back and edit and try to add more tags. Experience none of the new tags showing up in the preview. BUG

3.
- Attempt to rename the existing tag too. Now you get Fatal error: Call to a member function uri() on a non-object in .../core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/LinkFormatter.php on line 46

The code that was ported from Drupal 7 seems to have been ignoring that terms that does not exist yet might be somehow on the entity. The logic to maintain the display of 'autocreate' terms (where this value is used for the tid) is there, but there is nowhere to create the autocreate tid. The word 'autocreate' only appears in conditions and comments in core and no assignment is done with this value as far as I've found.

Proposal

Figure out how to pass around these new terms properly. Don't filter them out on a whim. Write tests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

As said the rendering code in Drupal\taxonomy\Plugin\field\formatter\LinkFormatter, etc. are full of conditions for terms with 'autocreate' as their tid, but the actual execution never reaches there, the tid that gets assigned is FALSE. That is coming from Drupal\taxonomy\Plugin\field\widget\TaxonomyAutocompleteWidget's massageFormValues() method. For some reason the temporary terms it creates get 'tid' => FALSE. Just fixing this does not solve the problem though, because these get filtered out before they reach display anyway.

Dave Reid’s picture

Gábor Hojtsy’s picture

Indeed, that does do away with 'autocreate' as it seems, however it did not have a patch in over a month, and definitely does not apply anymore, so impossible to check ATM.

Gábor Hojtsy’s picture

Priority: Normal » Major

Elevating to major given that trivial user input results in fatal error. There are clear gaps in test coverage... :/

Gábor Hojtsy’s picture

Priority: Major » Normal

This is insane. I attempted to write a test case to demonstrate the problem, but it does not appear in the test case I wrote... Then I went to simplytest.me to reproduce this and I could reproduce it like the following but no WSOD there :O

1. if the node already had terms, no new added autocomplete term showed up on preview, only the existing terms were previewed
2. if the node had no terms or I changed the input to only contain new terms, they all showed up

Obviously 1. is a bug. I could not reproduce the WSOD there, so moving down to normal. Also while I can reproduce 1 on simplytest.me and locally manually, I could not reproduce 1 in a test case, which literally grew me such a headache I now need to go and brew some tea and sit down in a quiet corner for a short while.

Gábor Hojtsy’s picture

Here is the test I attempted to prove the bug with. As said, I can reproduce manually but failed to reproduce with the test (it is passing for me, heh) :/

All I attempted to do is to change the existing node preview test to use an autocomplete widget instead of a select widget and then input different new/old tag combinations to reproduce my manual testing. That made the display of the field disappear from the node type (even on teasers), so I needed to add the instance to the display to make it appear again. That is strange, btw. This strange also seemed to swap out where its displayed, it is not displayed on teasers even though we have that set in the settings. Heh.

First it uses an existing term and then it tries to use new terms entirely. So no reproduction for (3). And then it tries to combine old and new to reproduce (2).

Anyway, the test passes for me, but I can reproduce the bug on simplytest.me at least (2) that is. I cannot reproduce (3) anymore. Screenshot of bug from simplytest.me:

Screenshot_5_6_13_10_44_AM.png

Gábor Hojtsy’s picture

(Take the second patch from above, that is more complete).

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Continuing the debugging of this is the next thing on my plate.

Berdir’s picture

Status: Active » Needs review
FileSize
6.62 KB

That's actually not caused by the term NG conversion but the term reference field NG conversion I think.

And yes, looks like we don't have test coverage for quite a few things.

autocreate is gone, instead, $item['entity'] is just there and tid is false.

Gábor Hojtsy’s picture

@Berdir: woot, thanks! So do you have a tip on why did my tests not find this bug? It does not look like a bug that would need more conditions to apply :/

Berdir’s picture

This test should fail nicely.

As discussed in IRC, the test hit another special case, when there are only new terms, then the loop in TaxonomyFormatterBase::prepareView() doesn't run at all and no items are removed that it thinks are empty. And because $item['entity'] is set and tid is not autocreate, it thinks it has a real term and tries to display it as a link, which kinda works except there's no id, so the link is broken.

Extended the tests so that it uses an existing term in one case and also added asserts as to when terms should be linked and when not.

Status: Needs review » Needs work

The last submitted patch, taxonomy-autocreate-1985138-11.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Berdir is kicking ass on this and is far more knowledgeable than me in this area, so unassigning.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.3 KB

This should fix those tests.

Berdir’s picture

Forgot the interdiff.

Status: Needs review » Needs work
Issue tags: -Regression, -sprint, -Spark

The last submitted patch, taxonomy-autocreate-1985138-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +sprint, +Spark
Berdir’s picture

Random test failure, database related.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Indeed, we already have the entity property containing the runtime entity for non-saved new ones. Thanks for providing a better test as well :) Looks good to me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.