- Issue created by @joachim
- Assigned to joachim
- π¬π§United Kingdom joachim
I'm working on this.
I really think this needs to get into 10.3 as it's a regression.
- π¬π§United Kingdom joachim
Urgh, I was hoping to add a quick and dirty hack to FieldTypePluginManager, but FieldStorageAddForm calls $this->fieldTypeCategoryManager->getDefinitions() as well, so it still finds non-existent categories.
- π¨πSwitzerland berdir Switzerland
This is by design, they are not meant to be supported. categories are their own definition now, it's not just a string.
Several modules, including paragraphs and simplenews have examples on how to be compatible with both 10.2 and earlier versions. Plugin needs to be updated too.
- π¬π§United Kingdom joachim
Until contrib modules do this update, they make the Field add form look broken. It's particularly bad with Plugin, because that module adds 20+ field types:
Do you mean this code in Paragraphs: https://git.drupalcode.org/project/paragraphs/-/blob/8.x-1.x/paragraphs....
That's a LOT of code to have to add to be compatible with both 10.1 and 10.2.
When you consider that for changing a test trait we add BC handling with a deprecation message, the BC handling around the change to field type categories is REALLY bad. We should be fixing this.
- π¨πSwitzerland berdir Switzerland
plugin is a special case, I don't think any other module add more. For most it's 1-3 or so and you really need to rethink the category to fit into the new UI.
Paragraphs is a special case, it's not not a field type, it's a predefined option that is being promoted up, like media. The basic example is this: https://git.drupalcode.org/project/dynamic_entity_reference/-/blob/3.x/d....
We have BC handling, it doesn't break and it adds it to general. I don't think we should spend time in core to do more, because there is no straight forward conversion. It's a completely different UI and most modules that use a category will need to rethink that and instead consider to use an existing category. Plugin could also be seen as a reference, only the fact that there are 20 of them (which probably really shouldn't work like that, it should most likely be a setting, like entity references but there might be a technical reason why that's not possible, I don't remember) is a reason to have its own category.
- Status changed to Needs review
8 months ago 1:07pm 22 April 2024 - π¬π§United Kingdom joachim
> We have BC handling, it doesn't break and it adds it to general.
But it looks like craaaaaaaaaaaaaaaaaaap.
And like I say, we normally nitpick BC to the TINIEST detail, so why not here?
I've got a fix that works. It's hacky but simple, and I don't think that it matters that it's hacky as it gets removed for 11.x anyway.
Pushing the fix to an MR, since you seem dead set against it I'm not bothering with cleaning up the code :/
- π¨πSwitzerland berdir Switzerland
I'm not "dead set" against it, I just think it's not worth your time and is unlikely to get committed. This isn't a 10.3 regression, it's already in 10.2.
Yes, plugin looks bad, but it can be improve to look better in both 10.1 and 10.2 fairly easy and again, plugin is an exceptional case.
- Status changed to Needs work
8 months ago 5:58pm 26 April 2024 - πΊπΈUnited States smustgrave
If we do change this seems like we should have test coverage for it.
- π¬π§United Kingdom joachim
Do we really need tests for something that's going to be removed from 11.x?