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

Document and discuss route-path-to-function-name rename mapping #258

Open
kadamwhite opened this issue Dec 2, 2016 · 4 comments
Open

Document and discuss route-path-to-function-name rename mapping #258

kadamwhite opened this issue Dec 2, 2016 · 4 comments
Labels

Comments

@kadamwhite
Copy link
Collaborator

As noted in #257 by @gambry, we make some pretty broad assumptions about how to map route identifiers and post type names into setter names—converting for example (?P<revision_id>[\d]+) into a camel-cased setter method .revisionId(). #257 adds support to map kebab-case identifiers into the same camelCasing, but @gambry notes

Ideally the exact regexp should be [^a-z]+, in order to cover any future changes and plugins creating weird endpoints.

This issue is a placeholder for discussion of whether the regex should be changed for broader future compatibility, although the current regexp should account for all valid CPT identifiers and for the names in PCRE named capture groups. (Note that we would need [^a-z0-9], as both identifiers support alphanumeric strings, not just alphabetical strings).

However this remapping can be considered slightly "magical", so this issue can also serve to discuss whether this behavior is still desirable, or if there are better ways to enable consumers of the library to map values in a more powerful way.

@gambry
Copy link
Contributor

gambry commented Dec 6, 2016

[^a-z0-9] looks like the right range, according with register_post_type() valid $post_type values.
However the documentation seems to allow custom types starting with a number, which could result in mapping these routes to methods starting in the same way BUT javascript doesn't allow variables starting with numbers.

So introducing [^a-z0-9] needs an additional condition to check if the route starts with numbers and replace/remove them too until the first alphabetic character is found.

@kadamwhite
Copy link
Collaborator Author

That starts-with-number issue is a good catch, thank you for the comment! I can make that change in a few hours but if you have the time to submit a PR it'd be welcome too!

@kadamwhite
Copy link
Collaborator Author

Upon reflection, a setter starting with a number is valid, it's just cumbersome: assuming the capture group name 1more_time, the setter could be accessed with bracket notation site.myResource()[ '1moreTime' ].... For this reason I'm hesitant to make this logic even more "magical" without input from a wider group of users about what their expectations would be.

@gambry
Copy link
Contributor

gambry commented Dec 12, 2016

I agree. Although we are discussing about weird edge cases, having wider thoughts is the only way to do things right.

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

No branches or pull requests

2 participants