Closed (fixed)
Project:
Custom Elements
Version:
3.x-dev
Component:
Code
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
26 Apr 2024 at 15:45 UTC
Updated:
23 May 2024 at 14:19 UTC
Jump to comment: Most recent
Problem/Motivation
Auto plugin should do regular processing, but needs a configurable attribute/slot name. For that we need to improve our processor pipeline to add support for this optional name. The name should be optional, to keep BC for 2.x processors.
Issue fork custom_elements-3443794
Show commands
Start 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
fagoComment #3
roderikOK. I'm fairly sure what should roughly be done here, but the code is too abstract for me to see if it's small or large.
I'll just start by hacking the optional name argument into CustomElementGenerator::process() + the addToElement() implementations, starting with DefaultContentEntityProcessor + DefaultFieldItemListProcessor.
And then see if I have questions / whether I already see what the actual practical behavior change is and how AutoCeFieldFormatter::build() should be simplified (??) when it works...
Comment #5
roderik( OK that wasn't actually very much / didn't cause any breakage. I just needed to change/test the code, in order to see things clearly. )
Created draft change record. Created MR with question about whether we should add an extra annotation.
Comment #6
fagohmm, this is not so "optional" as I had thought of it.
That's not good, since it could result into severe upgrade headache. e.g. at lupus_decoupled we want to support custom_elements 2 and 3 in parallel until 3 is stable. Also that means custom code must be changed when updating, what makes it complicated. I think we should check and find a way to avoid this.
>The name should be optional, to keep BC for 2.x processors.
We really need this BC I think.
Comment #7
fagoTook a stab on how it could look like in a fully BC way. What do you think about that?
https://git.drupalcode.org/project/custom_elements/-/merge_requests/49/d...
Comment #8
roderikThat proposal looks good.
Because of the other potentially-breaking changes we are introducing, I was getting lost a little on whether we want to prevent breakage for sites on the 3.x upgrade. But personally I always tend to favor 'not breaking things' so I agree. (And yes, this change would break things in a 'wider' way than the others.)
I amended the Change Record. I also added a note that things still break on upgrade for Processor classes which extend custom_elements processor classes -- but that's not an official contract.
I might want to sneak a sentence into CustomElementProcessorWithKeyInterface comments, referring to the reason for needing to implement it (see Change Record). But I don't think I have push rights to your MR in the main repo, so I'll do that in another MR.
So:
RTBC for code
Needs Review for change notice.
Comment #9
roderikOh -- I guess you wanted me to add your code to MR 48 because that's the one referenced here.
So I did, and I did sneak in that extra sentence. (I believe that there's plenty of room for code readers to get confused between Processors and Formatters and CE displays etc - so a minimal reference to "why?" is IMHO warranted.)
Technically it refers to a specific implementation detail, so I'll remove it if you don't like that.
https://git.drupalcode.org/project/custom_elements/-/merge_requests/48/d...
Comment #10
fagoComment #11
roderikMerged 3.x (includes #3445143: Separate configuration of CE field formatters and display components), implemented feedback, build is green again.
Comment #13
fagoI resolved conflicts with gitlab and merged results! Thanks!