Skip to content

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Aug 27, 2024

Summary

Move legacy OC_Image into \OC\Image and update code style and remove deprecated references in there.
This class is exposed through OCP\Image which is used by applications with new \OCP\Image() and then \OCP\IImage methods can be used on it.

This still leaves a reference to OC from OCP which we try to avoid.
But this would mean adding an IImageFactory which applications needs to DI, not sure this is worth it.

Checklist

@come-nc come-nc added 3. to review Waiting for reviews technical debt labels Aug 27, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Aug 27, 2024
@come-nc come-nc self-assigned this Aug 27, 2024
@come-nc come-nc force-pushed the fix/move-image-to-oc-namespace branch from 17dec99 to 0010f28 Compare August 27, 2024 15:35
@come-nc come-nc requested review from a team, artonge, icewind1991 and sorbaugh and removed request for a team August 27, 2024 15:57
@come-nc come-nc force-pushed the fix/move-image-to-oc-namespace branch from f296d19 to 1f46be7 Compare August 29, 2024 15:06
@come-nc come-nc merged commit e44f24f into master Aug 29, 2024
@come-nc come-nc deleted the fix/move-image-to-oc-namespace branch August 29, 2024 16:01
@provokateurin
Copy link
Member

provokateurin commented Sep 2, 2024

This broke Talk nextcloud/spreed#13197 and using the OCP replacement does not work.
I think the problem is the \OCP\Image depends on \OC\Image which does not exist in the dependency and thus all methods are missing.
Also some of the methods are only in \OC\Image and not in \OCP\Image or \OCP\IImage and thus are not visible.

To fix this \OCP\Image needs to implement \OCP\IImage directly and the missing method need to be added to \OCP\IImage.

@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants