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

fix: DefinedRouteCollector.php to use RouteCollectionInterface #8206

Closed
wants to merge 1 commit into from

Conversation

webalchemist
Copy link
Contributor

@webalchemist webalchemist commented Nov 14, 2023

Use RouteCollectorInterface rather than RouteCollector to allow for people implementing new RouteCollectors by implementing the interface as intended.

Currently this would generate type errors.

Referenced in <#8204>

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Use RouteCollectorInterface rather than RouteCollector to allow for people implementing new RouteCollectors by implementing the interface as intended.

Currently this would generate type errors.
@kenjis
Copy link
Member

kenjis commented Nov 14, 2023

Don't remove the PR template.

@webalchemist
Copy link
Contributor Author

Sorry, I'm not really familiar with the process. Just trying to help.

@kenjis
Copy link
Member

kenjis commented Nov 14, 2023

Okay, thank you for trying to improve CI4.
We use the checklist to check the PR.

Unfortunately, this issue is not easy to fix.
Because RouteCollectionInterface is broken, so if you fix the DefinedRouteCollector,
the following PHPStan errors are reported.

Error: Method CodeIgniter\Router\RouteCollectionInterface::getRoutes() invoked with 1 parameter, 0 required.
Error: Call to an undefined method CodeIgniter\Router\RouteCollectionInterface::getRoutesOptions().
Error: Call to an undefined method CodeIgniter\Router\RouteCollectionInterface::getRoutesOptions().
 ------ ------------------------------------------------------------------ 
  Line   system/Router/DefinedRouteCollector.php                           
 ------ ------------------------------------------------------------------ 
  50     Method CodeIgniter\Router\RouteCollectionInterface::getRoutes()   
         invoked with 1 parameter, 0 required.                             
  55     Call to an undefined method                                       
         CodeIgniter\Router\RouteCollectionInterface::getRoutesOptions().  
  60     Call to an undefined method                                       
         CodeIgniter\Router\RouteCollectionInterface::getRoutesOptions().  
 ------ ------------------------------------------------------------------ 


Error:  [ERROR] Found 3 errors     

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6858472463/job/18649336485?pr=8206

@webalchemist
Copy link
Contributor Author

It looks like RouteCollectionInterface hasn't been updated along with the changes to RouteCollection either. There are some additional functions and some changes to function parameter lists.
Nothing else within the framework implements RouteCollectionInterface so it should be safe to update.

@kenjis
Copy link
Member

kenjis commented Nov 14, 2023

If the core team agrees, it would be possible to fix the interface in 4.5 branch.
I personally am in favor of fixing it.

But we do not accept a interface change in develop branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 14, 2023
@kenjis kenjis changed the title Update DefinedRouteCollector.php to use RouteCollectionInterface fix: DefinedRouteCollector.php to use RouteCollectionInterface Nov 14, 2023
@kenjis kenjis added the wrong branch PRs sent to wrong branch label Nov 14, 2023
@MGatner
Copy link
Member

MGatner commented Nov 23, 2023

How about adding a new interface in 4.5 instead, then using that for the parameter? Slightly less breakage, and as long as the class implements both interfaces (or maybe the new one extends the old?) it shouldn't affect 99% of devs.

@kenjis kenjis added the abandoned? Activity on a pull request is almost none since the last interaction with the author label Mar 11, 2024
@kenjis kenjis added the stale Pull requests with conflicts label Apr 7, 2024
@kenjis
Copy link
Member

kenjis commented May 24, 2024

I sent another PR #8911

@kenjis kenjis closed this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned? Activity on a pull request is almost none since the last interaction with the author bug Verified issues on the current code behavior or pull requests that will fix them stale Pull requests with conflicts wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants