Skip to content

adjust request matcher to the symfony model #36

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

Merged
merged 1 commit into from
Mar 4, 2016

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Mar 2, 2016

fix #33

this matcher has less options than the Symfony request matcher. a Symfony request is more like the ServerRequestInterface of psr 7 and we mainly work with client requests, so matching on the client ip makes no sense. and psr 7 requests have no attributes so i dropped that as well.

@dbu dbu force-pushed the refactor-request-matcher branch 2 times, most recently from e301446 to ad41561 Compare March 2, 2016 09:39
@sagikazarmark
Copy link
Member

Isn't this a BC break? I think that message has already been tagged by @joelwurtz

@dbu
Copy link
Contributor Author

dbu commented Mar 2, 2016

code using the matcher would still "work" but the meaning of the first parameter has changed. it used to match anything of the uri: scheme, host or path. now it only applies to path. not sure if that could be a 1.2 version or is really too bad a BC break.

not sure how we should handle this. i don't want to add a separate class that does the same but different. should we delay merging this until we bump to 2.0?

@sagikazarmark
Copy link
Member

Well, we either add a separate one which does the same and deprecates the other (and gets removed in 2.0) or just do this and break BC in 1.2. That would require a big fat warning.

From a BC point of view the first one seems to be better to me. Although I am not sure how many people might already use this feature (but this mentality is not the best for versioning).

@dbu dbu force-pushed the refactor-request-matcher branch 3 times, most recently from 2a4a70a to 1271fe7 Compare March 2, 2016 10:35
@dbu
Copy link
Contributor Author

dbu commented Mar 2, 2016

okay, you are right. we do semver, we should not randomly decide to violate BC.

deprecated and add a new one as RequestMatcher. what do you think of this?

@sagikazarmark
Copy link
Member

Sounds like a plan

@sagikazarmark sagikazarmark force-pushed the refactor-request-matcher branch from 1271fe7 to ade8f31 Compare March 4, 2016 19:53
@sagikazarmark sagikazarmark force-pushed the refactor-request-matcher branch from ade8f31 to 671d9d7 Compare March 4, 2016 19:55
sagikazarmark added a commit that referenced this pull request Mar 4, 2016
Add Symfony port of RequestMatcher, closes #33
@sagikazarmark sagikazarmark merged commit a97bd29 into master Mar 4, 2016
@sagikazarmark sagikazarmark deleted the refactor-request-matcher branch March 4, 2016 20:54
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.

Replace regex/add symfony-like request matcher
2 participants