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

Allow extra keys for certain configuration #1484

Merged
merged 2 commits into from
Sep 26, 2022
Merged

Conversation

bobvandevijver
Copy link
Contributor

@bobvandevijver bobvandevijver commented Sep 6, 2022

This makes it possible to configure the bundle using the Symfony way

Q A
Branch? 2.x
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
License MIT
Doc

Symfony generates a configuration class for bundle configuration, which can be used to configure the bundle with php and IDE autocomplete magic. For this bundle, that main class would be Symfony\Config\LiipImagineConfig, which is located in your cache directory.

However, currently it is not possible to configure the filter properties, as extra keys are not explicitly allowed. This MR aims to solve this.

Before this MR, the generated class for the filter configuration would look like:

<?php

namespace Symfony\Config\LiipImagine\DefaultFilterSetSettings;

use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;

/**
 * This class is automatically generated to help in creating a config.
 */
class FiltersConfig 
{
    public function __construct(array $value = [])
    {
        if ([] !== $value) {
            throw new InvalidConfigurationException(sprintf('The following keys are not supported by "%s": ', __CLASS__).implode(', ', array_keys($value)));
        }
    }

    public function toArray(): array
    {
        $output = [];

        return $output;
    }
}

After this change, it will look like:

<?php

namespace Symfony\Config\LiipImagine\FilterSetConfig;

use Symfony\Component\Config\Loader\ParamConfigurator;

/**
 * This class is automatically generated to help in creating a config.
 */
class FilterConfig
{
    private $_extraKeys;

    public function __construct(array $value = [])
    {
        $this->_extraKeys = $value;
    }

    public function toArray(): array
    {
        $output = [];

        return $output + $this->_extraKeys;
    }

    /**
     * @param ParamConfigurator|mixed $value
     * @return $this
     */
    public function set(string $key, $value): self
    {
        $this->_extraKeys[$key] = $value;

        return $this;
    }
}

This ultimately allows configuration like this:

<?php

use Symfony\Config\LiipImagineConfig;

return static function (LiipImagineConfig $liipImagine): void {
  $profileImageIcon = $liipImagine->filterSet('profile_image_icon')->quality(60);
  $profileImageIcon->filter('auto_rotate');
  $profileImageIcon->filter('fixed', ['width' => 25, 'height' => 25]);
  // or, depending on preference
  $profileImageIcon->filter('fixed')
    ->set('width', 25) 
    ->set('height', 25);
};

Without this change, you would get an error like:

The following keys are not supported by "Symfony\Config\LiipImagine\FilterSetConfig\FilterConfig": width, height

@coveralls
Copy link

coveralls commented Sep 6, 2022

Coverage Status

Coverage decreased (-2.09%) to 79.388% when pulling 283b37f on bobvandevijver:patch-1 into 747966c on liip:2.x.

@dbu
Copy link
Member

dbu commented Sep 26, 2022

hi, thanks for this PR. can you please rebase on 2.x to get rid of the codestyle errors?

and can you please add an entry to the changelog, for the next minor version?

i am not too familiar with the extra keys thing - are we sure this will not be a BC break for existing users? i think it won't break anything existing, but could you confirm?

This makes it possible to configure the bundle using the Symfony way
@bobvandevijver
Copy link
Contributor Author

@dbu Requested changes have been done!

About impact: it shouldn't impact existing users, or at least, not that I know of.
I have tested this change with my project which has about 10 filters configured. The output of bin/console debug:config liip_imagine is identical with both versions.

Only the order of the keys becomes different when you start using the new php configuration that this PR enables, so when you use $profileImageIcon->filter('fixed', ['width' => 25, 'height' => 25]); instead of $container->extension('liip_imagine', ['filter_sets' => ['profile_image_icon' => ['filters' => ['fixed' => ['width' => 25, 'height' => 25]]]]]).

@dbu dbu merged commit 8f939d3 into liip:2.x Sep 26, 2022
@dbu
Copy link
Member

dbu commented Sep 26, 2022

great, thanks a lot for this improvement!

@bobvandevijver bobvandevijver deleted the patch-1 branch September 26, 2022 15:07
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.

3 participants