Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Function preg_match not working properly #26

Closed
WillyReyno opened this issue Sep 13, 2018 · 6 comments
Closed

Function preg_match not working properly #26

WillyReyno opened this issue Sep 13, 2018 · 6 comments

Comments

@WillyReyno
Copy link

WillyReyno commented Sep 13, 2018

Method preg_match doesn't seem to be working properly after installing Safe.

I made a regex to split spotify URL and retrieve the id. Here is my code which is working when I remove Safe's use function :

public function getTrackIdFromUrl(string $url): ?string
    {
        $spotifyRegex = "/https?:\/\/(?:embed\.|open\.)(?:spotify\.com\/)(?:track\/|\?uri=spotify:track:)((\w|-){22})/";
        preg_match($spotifyRegex, $url, $matches);
        return $matches[1] ?? null;
    }

For the url https://open.spotify.com/track/0nCqpKBrvDchO1BIvt7DTR?si=iLUKDfkLSy-IpnLA7qImnw the return of $matches[1] should be 0YCRJcAXQ3NSfKN1x1IXK3.
With Safe, $matches returns null.

Thank you for the awesome lib :)

@mallardduck
Copy link

Give this a shot here:

public function getTrackIdFromUrl(string $url): ?string
    {
        $spotifyRegex = "/https?:\/\/(?:embed\.|open\.)(?:spotify\.com\/)(?:track\/|\?uri=spotify:track:)((\w|-){22})/";
        $matches = [];
        preg_match($spotifyRegex, $url, $matches);
        return $matches[1] ?? null;
    }

@WillyReyno
Copy link
Author

Well, that was simple. Thank you!

However I don't know if the package should change the behavior of the original method (even though it was a bit magic)?

@mallardduck
Copy link

Sure thing, glad I could help!

I haven't tested it myself - yet - however I think it may be a behavior that's unavoidable when shimming this type of function. I'll test it for real and then report back.

@mallardduck
Copy link

So after a bit of testing I found out that you're correct in that this change in behavior could be avoided. I'm not sure how that would be done since I'm not familiar with the generation of the shims - but changing the behavior here should be avoidable for sure.

Essentially just turning the last elseif into the else used would fix it.

@moufmouf
Copy link
Member

Hoooo I see... Thanks @WillyReyno and @mallardduck for pointing this out.

Just reproduced the error in a unit test. The issue arises with all functions that have optional "by reference" arguments.

I'm trying to implement @mallardduck fix in the code generator right now.

@moufmouf
Copy link
Member

....aaaaand it's fixed!

03998c2

:)

Thanks to all of you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants