Added TermForm::getParentIds for allowing to override in contrib

I wasn't sure if the patch or the MR was the latest here, so I kicked the tires on the two MRs now that the CI outage is over.

Thanks for working on this. This looks like a very sensible fix; I'm signing off on the method addition in my little-used role as Taxonomy subsystem maintainer. 🪄😀

Promoting to major, since it's a contrib blocker and a regression.

I put a couple of nitpick suggestions on the MR. The main thing we still need here though is a change record. Once that's added, this can be committed to 10.1.x.

Typically, API additions are only allowed in minor releases, even for bugfixes. However, I think there's a low risk of method name collisions for this backport, and it's more important to fix the affected contrib in the production branches. I'll check with the other release managers to confirm that this is backportable.

We should also maybe add a regression test and a unit test for the new method?

Thanks for working on this!

Trying that one more time.

@xjm, Thank you for review. I see you tagged with "Needs tests". Hm, I don't see why as this is just moving around a piece of code from a method into a new method. Also, "Needs change record" and "Needs release manager review"? This is just minor change with no impact to API. We're creating a new protected method and is not API addition.

Or maybe the simplest change would be to remove the (int) as per #13

I think it is quite a lot to expect core and contrib to work when the type of the ID field is changing. The code referenced in #13 is for the entity reference field. It has not much to do with the data type as defined by \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions() which is what this is changing for taxonomy fields.

This is not just about this bit of code... it's also about things like:

entity.taxonomy_term.canonical:
  path: '/taxonomy/term/{taxonomy_term}'
  defaults:
    _entity_view: 'taxonomy_term.full'
    _title: 'Taxonomy term'
    _title_callback: '\Drupal\taxonomy\Controller\TaxonomyController::termTitle'
  requirements:
    _entity_access: 'taxonomy_term.view'
    taxonomy_term: \d+

Which I guess the rdf_entity is altering. I think we could consider saying if you altering all the other things then you can replace the entire form too. The change here seems okay in the tiny scope of make this work with core and the rdf_entity module but I'm not sure that the approach of the rdf_entity module is actually sustainable.

@alexpott, thank you, it makes sense.

@xjm, could we move with this w/o tests? It's just moving code around.

• Review remarks addressed
• IMHO, this is not an API change, we're just moving a piece of code in a protected method. So, I don't think this needs tests, change record or release manager review. I'm tentatively untagging to get more explanation for these tags.
• As I have doubts on tags and this being an API change, I'm settings back to RTBC to get more feedback from @xjm
• Given @alexpott comment from the tiny change is acceptable.

Updated issue summary to my best knowledge of what this patch is about now.

FWIW my opinion here is that but I'm not sure that the approach of the rdf_entity module is actually sustainable. - I think that swapping out the storage and changing the underlying type of the ID field is beyond to make it inoperable with some of contrib and probably other parts of core.

@alexpott

I think that swapping out the storage and changing the underlying type of the ID field is beyond to make it inoperable with some of contrib and probably other parts of core

There some risks, true. But since 2017 in production this is the 1st time it become inoperable with core.

I agree with #23 that given this is just adding a protected method to a form and moving code around it doesn't need explicit test coverage.

However, to backport it to 10.0.x and 9.5.x, we should add a change record, because it's a method addition and modules doing similar to rdf_entity might want to know about it.

@catch created https://www.drupal.org/node/3348454 →

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

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