New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4.0] Drop Framework's Image Package #25763
Conversation
libraries/src/Image/ImageFilter.php
Outdated
// If a logger hasn't been set, use DelegatingPsrLogger | ||
if (!($this->logger instanceof LoggerInterface)) | ||
{ | ||
$this->logger = Log::createDelegatedLogger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, I removed logger injection from constructor and changed this (and Joomla\CMS\Image\Image::getLogger()
) to return Joomla\CMS\Log\DelegatingPsrLogger
as opposed to Psr\Log\NullLogger
. To me it didn't make sense to inject one and then default to another. But if this is an unnecessary change, it can be reverted and instead we could check in the constructor if the logger is not set to still allow custom loggers in constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure Image::getFilterInstance()
injects the logger (whether by constructor or the logger aware interface's setLogger()
method doesn't matter). Then you don't need this fallback. I realize the current code doesn't do that, but maybe the code can finally be done right. Because realistically, if the Log::createDelegatedLogger()
facade wasn't available, you wouldn't be able to do this, and you're left either creating the NullLogger
or fixing all the code that assumes a logger is always available in the class (we can leave opinions about whether a getLogger()
method in a random class should actually be setting a default logger or not at the door).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using setLogger()
after instantiating filter class wouldn't work because it already needs a logger in the constructor. Please review 9c5f894.
Test is failing:
No idea how to fix this. It expects folder creation to fail but that succeeds. |
That's not true, Media manager is handling ALL the edit plugins, at least in the edit view, in the client-side (browser) |
@dgrammatiko Media Action - Resize plugin does use Image class to resize images, if plugin parameters are set. I'll update testing instructions. |
@SharkyKZ that's a huge thing then because no matter what dimensions you resize in the browser, those will be overridden from some parameters hidden in a plugin. I'm pretty sure the code exists only as a showcase that you can have plugins hooked on some PHP event but well I don't remember anymore. Maybe @laoneo remembers |
@SniperSister @zero-24 can someone have a careful look at rips please |
where we are on this cc @release-joomla |
Thanks! |
can you check Joomla\Image\Image::watermark() Thanks! |
Pull Request for Issue #22817 .
Summary of Changes
This brings back Framework's Image library back to CMS.
Testing Instructions
Use Image library in some ways. E.g. set width and height options in
Media Action - Resize
plugin,then use resize function in Media Manager.Expected result
Still works.
Todo:
Documentation Changes Required
Yes.