Remove code duplication in ImageAdapters

I 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

Yes @danielveza, I just pushed upstream changes to fix the tests.

PR looks good for RTBC.

Nice clean-up, thank you!

I'll rebase and merge this after 📌 Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component Fixed lands 👍

📌 Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component Fixed landed.

I'll merge in upstream tomorrow unless someone beats me to it. Meetings the rest of today…

@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! 😊)

I 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?

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.

I 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.

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.

Correct. It's a trait serving a narrow use case, and increases DRYness. Isn't that what you were aiming for? :D

Maybe if we go the trait route we narrow the scope a little and call it something like ImageAdapterTrait?

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 File entities, another for Media entities, yet another for User entities, etc. This trait can be valuable for all of those scenarios?

it still allows contrib/custom code subclassing rather than copying, hence it still is a de facto API.

I 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

Great! 😄

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 😊

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