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

Add UID route token #3583

Merged
merged 9 commits into from
Dec 31, 2018
Merged

Conversation

AugustMiller
Copy link
Contributor

Addresses #3582.

Sorry if this is too presumptuous—had the changes queued locally, so I figured it was worth committing!

✌️

@brandonkelly
Copy link
Member

@AugustMiller Have you tested both of these regexes with capital letters?

@angrybrad if UUIDs are only including letters a-f, we should probably update StringHelper::isUUID() to match this regex. (Also not sure why the u modifier is in there… a-f definitely won’t require that.)

@AugustMiller
Copy link
Contributor Author

In its current state, it won't match capital letters… all the references I've seen to the pattern assume lower-case a-f.

My intent was to match whatever is being output by StringHelper::UUID(), not necessarily cover outside world implementations.

Up to this point, I hadn't realized the “audit columns” were a Craft ActiveRecord extension, and not something that Yii provided!

@angrybrad
Copy link
Member

v4 UUID's allow for upper or lowercase, so the i flag should cover that. u is probably unnecessary. I've cleaned up and tightened the regex in this commit: 956b435

@angrybrad
Copy link
Member

@AugustMiller can you check if the functionality still works as you'd expect with those changes?

@AugustMiller
Copy link
Contributor Author

AugustMiller commented Dec 31, 2018

Unfortunately not—probably due to the subset of regular expressions that can actually be injected into a route?

For example, the final route (taken from our app) ends up as this, in the Yii route debugger:

account/membership/history/<subscriptionUid:/^[A-Z0-9]{8}-[A-Z0-9]{4}-4[A-Z0-9]{3}-[89AB][A-Z0-9]{3}-[A-Z0-9]{12}$/i>

…which is skipped by: account/membership/history/1a8e179d-3fc5-4fc5-ba52-85ccc286f5ad.

It seems like we might need an additional route-compatible pattern?

Edit: Some quick searching revealed nothing about case-insensitive route expressions in Yii—I wonder if the regex itself just needs to be doubly-verbose? The other challenge is the beginning-of-line and end-of-line flags, which may also be incompatible.

@angrybrad
Copy link
Member

@AugustMiller yeah, you're correct. This should do it though (actually tested it works with a UID route this time): cbd3abe

@AugustMiller
Copy link
Contributor Author

Works like a charm. 👌

@brandonkelly brandonkelly merged commit 654db51 into craftcms:develop Dec 31, 2018
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