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 setting custom options #12

Merged
merged 4 commits into from
Nov 27, 2023
Merged

Conversation

peterjaap
Copy link
Contributor

@peterjaap peterjaap commented Nov 26, 2023

Changing the visibility of the set() method in the OptionSet class from private to public allows us to set arbitrary settings using a text string.

For example, I want to be able to do this;

$customProcessingOptions = 'zoom:2:2/blur:4';
$options = array_map('trim', explode('/', $customProcessingOptions));
foreach ($options as $option) {
    $arguments = explode(':', $option);
    $name = array_shift($arguments);
    $url->options()->set($name, ...$arguments);
}

For a real life example, see elgentos/magento2-imgproxy@50f1455#diff-c5ca923b6d96fb8bb165a2634b824b6209bb35097555ff7cad065506f35b0c0cR77

peterjaap added a commit to elgentos/magento2-imgproxy that referenced this pull request Nov 26, 2023
@crocodile2u
Copy link
Owner

I understand the intent. I made it non-public on purpose: so that only known options be set in a controlled way. I suggest, instead of simply changing the set() method's visibility, to add a new method instead: setUnsafe() which will internally delegate to the same set(), of course. Would be nice to also have a unit test for this new method. What do you say?

@peterjaap
Copy link
Contributor Author

Perfect!

@peterjaap
Copy link
Contributor Author

@crocodile2u I've updated the PR with the testUnsafe method and a test

src/OptionSet.php Outdated Show resolved Hide resolved
@crocodile2u crocodile2u merged commit 7d267ca into crocodile2u:master Nov 27, 2023
@crocodile2u
Copy link
Owner

Thanks @peterjaap ! I'll create a new release now ;-)

@crocodile2u
Copy link
Owner

@peterjaap
Copy link
Contributor Author

@crocodile2u awesome, thanks for the quick turnaround and the constructive feedback!

@peterjaap peterjaap deleted the patch-1 branch November 27, 2023 14:59
WouterSteen pushed a commit to elgentos/magento2-imgproxy that referenced this pull request Feb 19, 2024
JeroenBoersma pushed a commit to elgentos/magento2-imgproxy that referenced this pull request Apr 4, 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