Skip to content

Conversation

@iRedds
Copy link
Collaborator

@iRedds iRedds commented Jun 6, 2023

Description
Currently the RedirectException class is in the CodeIgniter\Router\Exceptions namespace.
I believe this exception is not related to routes but to the HTTP layer. Therefore, I propose to move it to the appropriate namespace.
The old class is marked as deprecated.

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

@iRedds iRedds force-pushed the move-redirect-exception branch from 0891ccc to 6cffb87 Compare June 7, 2023 04:37
@kenjis kenjis added 4.4 breaking change Pull requests that may break existing functionalities labels Jun 7, 2023
@kenjis
Copy link
Member

kenjis commented Jun 7, 2023

If there is code that throws CodeIgniter\Router\Exceptions\RedirectException in apps,
it will break.

Why don't you catch the deprecated class in the framework code for backward compatibility?

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 7, 2023

Nice catch. Thank you.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@kenjis kenjis added the refactor Pull requests that refactor code label Jun 10, 2023
@kenjis kenjis merged commit efa0991 into codeigniter4:4.4 Jun 11, 2023
@kenjis
Copy link
Member

kenjis commented Jun 11, 2023

@iRedds Thank you!

@iRedds iRedds deleted the move-redirect-exception branch June 12, 2023 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Pull requests that may break existing functionalities refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants