2.0.2 breaks backward compatibility

Agreed. loadTerms() doesn't seem to account for multiple terms at all.

Additionally, it looks tome like 2.0.2 switched to using term UUID instead of term ID (#2695141), but provided no update hook for existing installations.

Additionally, it looks tome like 2.0.2 switched to using term UUID instead of term ID (#2695141), but provided no update hook for existing installations.

Yes, for some reason only changes to existing files from the original patch were committed, but not new files, which included a config schema and an update hook, which are now missing. To me, it doesn't look intentional, but like some kind of oversight. Note that the update hook only updates block configuration, but condition plugins can be used in other places as well. Some popular contributed modules using condition plugins coming to mind are rules, eca or menu_position. So even if the upgrade path had been committed, it might not have been sufficient for your use case. I think it might be hard to find and update all relevant configuration that might be using condition plugins, so this looks like an API break. In hindsight it might have been better to release this change in a new major instead of in a patch release. Also, I didn't yet look at the original patch closely to see if the array vs string issue would still need fixing even with the additional files committed.

💯 I'd add Context → and Asset Injector → to the list of popular contrib projects that use of condition plugins. No doubt there are more.

I think (hope) the configuration can still be updated — the update would need to check all config entries for a term_condition dependency and make the switch to uuid accordingly.

Updating Issue Summary and title, as this is bigger than Context and proper array handling.

Updating IS as it appears loadTerms() does correctly handle multiple terms. EntityStorageInterface::loadByProperties() expects and returns an array.

MR7 seems to work pretty well on an existing installation on our primary site. I'll be doing some additional testing on a clean install tomorrow, but wanted to open the MR so others could test/review if possible.

Patch attached if you're not into the GitLab thing.

Additional testing indicates the update hook Needs Work.

The latest updates to the MR:

- Changes the uuid config key to term_uuids to avoid over-writing config from the Context → module (a previous commit changed tid to uuid to better represent the data being stored).
- Removes invalid term condition configs (null tid values) — see 📌 Remove invalid configuration Closed: outdated .

I've tested sucessfully with configuration stored by blocks, Asset Injector and Context. Based on Menu Position's schema, it should work well there, too.

I'm not sure about config stored by ECA or Rules (ECA uses the 'conditions' key, but the structure is a bit different). Need some example config with which to test.

Also, the MR incorporates/builds on some of the work @RosK0 did in #9 🐛 Store UUID for exportability Fixed from 2695141 that didn't make it into the commit on that issue. If any of this makes it in, credit should be awarded accordingly.

Tested today on a clean instance of Drupal 9 and this is working as expected with term_conditions set on blocks, asset_injector, context and menu_position.

As mentioned above, if anyone is able to test with eca or rules config (or any other modules that use condition plugins), that would be helpful.

Otherwise, a review and/or feedback from the maintainers would be great. Thanks!

>  [notice] Update started: term_condition_update_9201
>  [error]  explode(): Argument #2 ($string) must be of type string, array given 
>  [error]  Update failed: term_condition_update_9201 

I'm running into the following when drush updb'ing patch #9

Sorry, there've been changes to the MR since I uploaded #9. Here's an updated patch.

If the update completed (e.g. drush updb says there's no update to run), you'll need to try again with a copy of the db that hasn't been updated, or you can reset term_condition's update schema with:

drush ev "\Drupal::keyValue('system.schema')->set('term_condition', (int) 9200)";

Thanks for working on the MR. This looks very good already! I have added a few inline comments, mostly not that important, but I think I have identified one case where the update hook still needs work.

I also wonder, if we should override ConditionPluginBase::calculateDependencies() and add additional dependencies on the term uuids, in case a term will be deleted? But that might be out of scope for this issue.

I tested the upgrade hook with menu_position upgrading on Drupal 10 from 2.0.1 to the MR and it seems to work well for that case.

Unfortunately I don't use eca or rules. At least for eca the tests have some examples, e.g. https://git.drupalcode.org/project/eca/-/blob/1.2.x/tests/modules/entity... So I think you are correct in that the existing update hook probably won't work for eca.

Thank you @FeyP for the excellent review. I'm working on some updates, especially regarding your point on supporting config created with 2.0.2!

There have been a handful of changes to term_conditions config over its life, but no update implementations so far. After testing each version, the format/changes are documented below to help in developing a comprehensive hook_update. Depending on what steps end users have taken, some or all of these config formats could still be hanging around.

8.x-1.0 – 8.x-1.1:

tid is scalar, with a single string value for the term (also possibly null — see below).

visibility:
  term:
    id: term
    tid: '1'
    negate: false

(Legacy config like the above is a likely cause of 🐛 In Term.php, there's an error if configuration's tid isn't an array Active .)

"Invalid" null values: Additionally, saving/re-saving any block with term_condition < 1.2 without configuring a term condition could result in config with tid: null, e.g.:

visibility:
  term:
    id: term
    tid: null
    negate: false

8x.-1.2 – 2.0.1:

tid is an array, target_id key added with each term id, no longer stores null. context_mapping key added.

visibility:
  term:
    id: term
    tid:
      -
        target_id: '3'
      -
        target_id: '4'
    negate: false
    context_mapping:
      node: '@node.node_route_context:node'

2.0.2

An array of term uuids is now stored in tid (target_id key removed) for new configurations only. Existing config remains as above.

visibility:
  term:
    id: term
    tid:
      - 1a63885a-106e-481a-bb9a-c3d064f98af9
      - bb2f8b21-8d3d-4671-82d7-286b763fc0f2
    negate: false
    context_mapping:
      node: '@node.node_route_context:node'

After another round of refactoring and testing, I feel like this is in a pretty good place. The update hook now deals with all the config formats/issues identified in #19 and removes config in cases where associated terms have been deleted.

Attaching the MR as a patch again to make it easy for folks to test.

Feedback from the maintainers, particularly with regard to changing the 'tid' key to 'term_uuids' would be helpful to know if we're headed in the right direction.

Maintainer here (although I haven't developed anything much on this project, only doing issue queue and release work), and the approach with regards to term_uuids looks excellent to me.

It would be good if we could get a review here and I can do another release today/tomorrow. We're unlikely to make things worse at this point :)

Patch #20 worked great for the few projects I've patched.

Since there hasn't been any further activity in the 2 weeks since RTBC, I have merged this issue in and am tagging a release.

Thanks @dieuwe!

@FeyP gave a pretty in-depth code review that, among other tweaks, prevented the update hook from blowing away new config created in 2.0.2. I think issue credit would definitely be merited if you're so inclined.

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