Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It seems that taxonomy terms are incorrectly affected to the 'Taxonomy upgrade extras' vocabulary in some cases.
As seen on the upgraded Drupal.org:
- http://drupalorg.staging.dev.damz.org/node/857776: Forum "News and announcements" correctly affected to the "Forum" field.
- http://drupalorg.staging.dev.damz.org/node/848062: Forum "Drupal showcase" incorrectly affected to the "Taxonomy upgrade extras" vocabulary, second term "Drupal 6.x" correctly affected to the "Drupal version" field.
Both nodes are of the same type (forum).
It looks like this could be a new beta blocker.
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedcatch asked for the content of {term_node} for those nodes.
Comment #2
catchHere's a patch for what I think the bug is.
To test this, looks like we need the following:
Vocabulary with single cardinality.
Node with at least two terms, the one that would be processed last needs to be the one in that single cardinality vocabulary.
After update, load the node and confirm that all the terms are in the right fields.
I won't be able to work on those tests for at least the next couple of hours, but may be able to look after that.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe bug can be reproduced with the upgrade path tests, we are just not asserting if the terms are associated with the correct vocabulary.
Here is an extension of the current test to test the fallback logic. This should fail.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe same patch with catch's fix. This one should pass.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #6
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedHi all,
on a purely philosophical standpoint (the '+1' looks weird and brings a tiny additional calculation), I would write:
rather than:
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commented@LaurentAjdnik let's rather stick with #4:
- this has to be consistent with http://api.drupal.org/api/function/field_default_validate/7
- the condition has to make sense: what we want to check is that the number of items is strictly bigger then the cardinality. The check in #4 is just that, the check in your #6 (while accurate and more efficient) doesn't carry out this.
Comment #8
catch#3 didn't fail.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt properly fails for me locally. I blame the test bot.
Comment #10
catchOK, going to test these locally and see.
Comment #11
chx CreditAttribution: chx commentedAt least the troubled line in the update needs more comments, "$delta[$field_name] +1" is the next delta and if that's more than the field cardinality........" or somesuch.
Comment #12
catchAdded an extra comment there.
Comment #14
catchwhitespace.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThou shall fail.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedIgnore #16, this one should fail in a more meaningful way.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis one should pass.
Comment #18
carlos8f CreditAttribution: carlos8f commentedNitpick: inconsistent spelling of "moreover", also seems a little repetitive.
Powered by Dreditor.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's spin-off the XPath issue: #933856: xpath() return values are inconsistent
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedWithout the hunks that got spined-off.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedSame as #21 with the text edits from #18.
Comment #23
carlos8f CreditAttribution: carlos8f commentedEr... #22 is identical to #21 :)
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry, here it is.
Comment #25
ksenzeeShould be "In our test database, each node is arbitrarily associated with all terms except two: one whose tid is ($nid), and one whose tid is (49 - $nid)."
"to the taxonomy extra field."
Ideally if ($should_be_displayed) would go first but I won't quibble.
With just the doc changes (no need to move the if clause) this is RTBC.
Powered by Dreditor.
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedDone.
Comment #27
ksenzeeLooks good. I verified that only comments have changed since the last testbot run (interdiff attached), so RTBC.
Comment #28
webchickWow, nice work, folks!
Committed to HEAD. Thanks!