The #2898903: Terms lose <root> as the parent when editing change broke the rdf_taxonomy sub-module of https://www.drupal.org/project/rdf_entity:
--- a/core/modules/taxonomy/src/TermForm.php
+++ b/core/modules/taxonomy/src/TermForm.php
@@ -23,7 +23,12 @@ public function form(array $form, FormStateInterface $form_state) {
$taxonomy_storage = $this->entityTypeManager->getStorage('taxonomy_term');
$vocabulary = $vocab_storage->load($term->bundle());
- $parent = array_keys($taxonomy_storage->loadParents($term->id()));
+ $parent = [];
+ // Get the parent directly from the term as
+ // \Drupal\taxonomy\TermStorageInterface::loadParents() excludes the root.
+ foreach ($term->get('parent') as $item) {
+ $parent[] = (int) $item->target_id;
+ }
$form_state->set(['taxonomy', 'parent'], $parent);
$form_state->set(['taxonomy', 'vocabulary'], $vocabulary);
Specifically, https://www.drupal.org/project/rdf_entity is swapping the storage of the taxonomy_term entity in order to store the terms in a triplestore backend. Along with this, the TIDs are strings (URIs), not integers. In #2898903: Terms lose <root> as the parent when editing they were casted to integers, prohibiting https://www.drupal.org/project/rdf_entity to use TermForm as it is.
Of course, https://www.drupal.org/project/rdf_entity can swap the form class handler and add its own but it has to copy/paste the entire TermForm::form() method.
N/A
Make it easier to extend TermForm::form() method by moving the parents computing in a protected method.
None.
None.
None.
None.
N/A
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
claudiu.cristeaComment #3
claudiu.cristeaComment #4
claudiu.cristeaTagging
Comment #7
claudiu.cristeaComment #8
claudiu.cristeaComment #9
claudiu.cristeaBecause of https://drupal.slack.com/archives/C51GNJG91/p1673438297534429 I'm posting also a patch to get testing
Comment #10
claudiu.cristeaRelated issue in affected contrib module.
Comment #11
claudiu.cristeaThe patch again
Comment #12
dimilias commentedSeems legit and it helps us with the non numerical IDs. RTBC +1.
Comment #13
penyaskitoI wasn't sure if this could be considered a bug, but from
core.data_types.schema.yml:So RTBC+1
Comment #14
penyaskitoAdjusting title
Comment #16
xjmI 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.
Comment #17
xjmThanks 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.
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 under a policy exception. If we do backport, it will need a release note.
We should also maybe add a regression test and a unit test for the new method?
Thank you!
Comment #18
xjmTrying that one more time.
Comment #19
claudiu.cristea@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 a minor change with no impact to API. We're creating a new protected method and that is not an API addition.
Comment #20
claudiu.cristeaOr maybe the simplest change would be to remove the
(int)as per #13Comment #21
alexpottI 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:
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.
Comment #22
claudiu.cristea@alexpott, thank you, it makes sense.
@xjm, could we move with this w/o tests? It's just moving code around.
Comment #23
claudiu.cristeaComment #24
penyaskitoUpdated issue summary to my best knowledge of what this patch is about now.
Comment #25
alexpottFWIW my opinion here is
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.
Comment #26
claudiu.cristea@alexpott
There are some risks, true. But since 2017 in production this is the 1st time it become inoperable with core.
Comment #27
catchI 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.
Comment #28
claudiu.cristea@catch created https://www.drupal.org/node/3348454
Comment #32
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!