Closed (fixed)
Project:
Experience Builder
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
4 Sep 2024 at 22:33 UTC
Updated:
3 Oct 2024 at 11:34 UTC
Jump to comment: Most recent
Overview
There is 3x Image adapter in experience builder, they all extend AdapterBase but only change one function. Each image adapter has it's own constructors and create functions that do the same thing. Lets treat ImageAdapter as a base and have the other two extend off that one.
I refactored these as part of a POC that didn't end up getting merged. Extracting those changes out to this issue.
Proposed resolution
Image Adapter is no longer a final class
ImageUrl & ImageAndStyle adaptors now extend ImageAdapter
User interface changes
N/A
Issue fork experience_builder-3472194
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
danielvezaComment #4
fazilitehreem commentedComment #6
danielvezaI won't RTBC since I did the original work, but confirming that the changes from @omkar-pd look the same as my original changes just with the latest commits from HEAD pulled in
Comment #7
omkar-pd commentedYes @danielveza, I just pushed upstream changes to fix the tests.
PR looks good for RTBC.
Comment #8
wim leersNice clean-up, thank you!
I'll rebase and merge this after #3450496: Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component lands 👍
Comment #9
wim leers#3450496: Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component landed.
I'll merge in upstream tomorrow unless someone beats me to it. Meetings the rest of today…
Comment #10
omkar-pd commentedComment #11
wim leers@danielveza I'm not a fan of making this class non-final just for the sake of DRY. I pushed a counterproposal: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... — what do you think?
(I'd like your +1 prior to merging! 😊)
Comment #12
danielvezaI think this is also works, I just have some thoughts below around the original approach & questions about the new trait :)
Having said that, I prefered the original approach. I think it was a perfectly valid and having the final constructor would help alleviate some of the concerns around making the ImageAdapter non-final.
The new trait is also very specific, it works if the class ONLY needs EntityTypeManager otherwise you need to either mess around or remove the trait to inject other services.
Maybe if we go the trait route we narrow the scope a little and call it something like ImageAdapterTrait?
Comment #13
wim leersI disagree — because it still means that the plugin class itself can be subclassed, just not the constructor. IOW: it still allows contrib/custom code subclassing rather than copying, hence it still is a de facto API.
Correct. It's a trait serving a narrow use case, and increases DRYness. Isn't that what you were aiming for? :D
How does that make things better? There's nothing image-specific about the trait.
All it does is make the entity type manager available, and that is something that lots of adapters may want to do. One may use it for interacting with
Fileentities, another forMediaentities, yet another forUserentities, etc. This trait can be valuable for all of those scenarios?Comment #14
danielvezaI can understand wanting to keep API scope small 👌.
The points you've made make sense to me and it does still achieve the goal of cleaning up the adapter code, so +1 that we merge it
Comment #16
wim leersGreat! 😄
Any area that you're looking to dig into next? 🤓 This cleans up an area we've just "proved it is possible", without pushing it further, and definitely without it appearing in the UI. It's so experimental/for later phase that it's not even part of https://git.drupalcode.org/project/experience_builder/-/blob/0.x/docs/da... 🤓 Did you see those docs already? That might lead you to an area you have strong opinions on 😊