Skip to content
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

ENH Allow devs to define support for third-party intervention drivers #610

Conversation

GuySartorelli
Copy link
Member

Provides an extension hook for defining which file types are supported for file conversions with third-party intervention/image drivers.

Issue

Comment on lines 102 to 109
// If the driver is somehow not GD or Imagick, we have no way to know what it might support
if ($driver !== 'gd' && $driver !== 'imagick') {
$supported = false;
$this->extend('updateSupportedByIntervention', $supported, $driver);
return $supported;
}
Copy link
Member Author

@GuySartorelli GuySartorelli Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there's no legitimate use case for updating the value or $supported if the driver is gd or imagick, since the logic below exactly mirrors the logic those drivers use internally.

@GuySartorelli GuySartorelli force-pushed the pulls/2/convert-with-3rdparty-driver branch from f402de8 to 1703ff5 Compare June 4, 2024 00:30
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads a little non standard, seems like it should something more like

$supportedDrivers =  ['gd', 'imagick'];
$this->extend('updateSupportedByIntervention', $supportedDrivers);

if (!in_array($driver, $supportedDrivers)) {
    return false;
}

@emteknetnz emteknetnz dismissed their stale review June 4, 2024 02:08

I misunderstood what this was doing

// If the driver is somehow not GD or Imagick, we have no way to know what it might support
if ($driver !== 'gd' && $driver !== 'imagick') {
$supported = false;
$this->extend('updateSupportedByIntervention', $format, $supported, $driver);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->extend('updateSupportedByIntervention', $format, $supported, $driver);
$this->extend('updateSupportedByIntervention', $supported, $format, $driver);

Change around the params to make it a little more intentful what this is for

We're updated $supported so I think it needs to be the first param as in convention

I think $format is probably next important because it most cases there's probably only one third party library, so won't need to if/else it, so it's less important that $format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of params for extension hooks isn't mentioned in our coding conventions... maybe it should be?
In this case I don't have any problem changing this order so I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@GuySartorelli GuySartorelli force-pushed the pulls/2/convert-with-3rdparty-driver branch from 1703ff5 to e63a581 Compare June 4, 2024 02:20
@emteknetnz emteknetnz merged commit db48540 into silverstripe:2 Jun 4, 2024
9 checks passed
@emteknetnz emteknetnz deleted the pulls/2/convert-with-3rdparty-driver branch June 4, 2024 22:32
GuySartorelli added a commit to creative-commoners/silverstripe-assets that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants