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 to modify options without creating a new object #133

Merged
merged 4 commits into from
Aug 16, 2016
Merged

Allow to modify options without creating a new object #133

merged 4 commits into from
Aug 16, 2016

Conversation

leofeyer
Copy link
Contributor

@leofeyer leofeyer commented Aug 12, 2016

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:

$slugify = $container->get('slugify');
$slugify->slugify('MY STRING', ['lowercase' => false]); // MY-STRING

TODO

  • Add the unit tests

If you decide that you want to implement this, I'll happily add the unit tests.

@florianeckerstorfer
Copy link
Member

This looks great. If you add unit tests, I will merge this.

{
foreach ($options as $key => $value) {
$this->setOption($key, $value);
}

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.

@leofeyer
Copy link
Contributor Author

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 @api tag, I figured you do not want to change it anymore?

@Toflar
Copy link

Toflar commented Aug 12, 2016

This will put the Slugify service into a state and affect consecutive calls. This is wrong. It would be way better to allow the slugify() method to accept parameters. This can even be achieved in a backwards compatible way:

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.

@florianeckerstorfer
Copy link
Member

You could also add a third parameter, options. Since this is BC we could do this in a 2.x release.

@leofeyer
Copy link
Contributor Author

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.

@florianeckerstorfer
Copy link
Member

You're right, I only thought about the implementation, not the interface. Yes, let's do it the way @Toflar described.

@leofeyer
Copy link
Contributor Author

I'll adjust the PR.

@leofeyer
Copy link
Contributor Author

I have updated the PR and also my initial comment to reflect the new usage.

@florianeckerstorfer
Copy link
Member

Thank you very much for this PR. Great work.

@leofeyer leofeyer deleted the patch-2 branch August 16, 2016 09:52
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.

None yet

3 participants