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

Try swapping in fastroute #51

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Try swapping in fastroute #51

wants to merge 9 commits into from

Conversation

Firehed
Copy link
Owner

@Firehed Firehed commented Jun 6, 2018

The regex-capture to fastroute-capture thing is a bit weird, but it's an ok first pass

This may fix #50 to some extent as well.

Needs WAY more testing and a way to set up caching and general configuration - or at least proper integration into the build_endpoint_list stuff

@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #51 into master will decrease coverage by 0.19%.
The diff coverage is 96.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #51     +/-   ##
===========================================
- Coverage     97.65%   97.46%   -0.2%     
- Complexity      100      109      +9     
===========================================
  Files            17       17             
  Lines           299      316     +17     
===========================================
+ Hits            292      308     +16     
- Misses            7        8      +1
Impacted Files Coverage Δ Complexity Δ
src/Dispatcher.php 97.47% <96.15%> (-0.57%) 45 <4> (+9)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3ef8ba...3b135bc. Read the comment docs.

@Firehed
Copy link
Owner Author

Firehed commented Sep 11, 2018

That most recent commit is currently broken, only due to some minor cleanup that needs to happen. It's directionally pretty close to the end-goal here (I think), pending some additional testing.

Good news: this routing separation helps with #53
Also good news: the crazy regex work is updated based on an actual application with some more-complex routes. It may be better to implement it differently, but does seem to correctly handle non-named capture groups and things of that nature.

Maybe-bad news: since FastRoute handles HEAD requests as it should (see #82), we will need to forcibly add some middleware to strip the body. Shouldn't be hard, but

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.

Add tools to help discover ambiguous routes
1 participant