-
Notifications
You must be signed in to change notification settings - Fork 252
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 to modify options without creating a new object #133
Conversation
This looks great. If you add unit tests, I will merge this. |
{ | ||
foreach ($options as $key => $value) { | ||
$this->setOption($key, $value); | ||
} |
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.
I would add a return $this
here.
Just so you know, this is just a workaround. It adds a state to the Symfony service, which one must not do. The correct solution would be this: interface SlugifyInterface
{
/**
* Return a URL safe version of a string.
*
* @param string $string
* @param array|null $options
*
* @return string
*
* @api
*/
public function slugify($string, $options = []);
} You would then pass the separator as follows: $slugify->slugify('My string', ['separator' => '_']); This is not backwards compatible and would therefore have to go into version 3. But since the interface has an |
This will put the Slugify service into a state and affect consecutive calls. This is wrong. It would be way better to allow the public function slugify($string, $options = null)
{
// BC (2nd parameter used to be the separator string)
if (is_string($options))
$separator = $options;
$options = [];
$options['separator'] = $separator;
}
$options = (array) $options;
// $options is now always an array and BC
} The interface can be changed without affecting anybody as the method signature does not change. |
You could also add a third parameter, |
You cannot add a third parameter to the interface without everyone implementing it getting a method signature exception. But the solution @Toflar described would work. |
You're right, I only thought about the implementation, not the interface. Yes, let's do it the way @Toflar described. |
I'll adjust the PR. |
I have updated the PR and also my initial comment to reflect the new usage. |
Thank you very much for this PR. Great work. |
Right now we need to create a new object instance every time we e.g. need a different regexp. This is cumbersome when using the library as a Symfony service, because it would require to define one service per configuration.
This PR adds methods to modify the options on the fly, so we can e.g. do this:
TODO
If you decide that you want to implement this, I'll happily add the unit tests.