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

Handlers types #3193

Merged
merged 4 commits into from
May 5, 2022
Merged

Handlers types #3193

merged 4 commits into from
May 5, 2022

Conversation

ashleycoles
Copy link
Contributor

Some more property type hints! This time for Handlers

A few notes on the changes made to ErrorHandler:

  • The method property was being accessed before init, so I've switched it to be a nullable string with a default of null
  • Both the logErrorDetails and displayErrorDetails properties were being accessed before init, so I've given them both default values of false

If there is a more sensible solution for the above, please let me know!

@coveralls
Copy link

coveralls commented May 4, 2022

Coverage Status

Coverage decreased (-0.0002%) to 99.892% when pulling 16af910 on ashleycoles:handlersTypes into f8f1377 on slimphp:4.x.

@l0gicgate l0gicgate added this to the 4.11.0 milestone May 5, 2022
@l0gicgate l0gicgate merged commit 187d99d into slimphp:4.x May 5, 2022
@dakujem
Copy link

dakujem commented May 29, 2023

Adding type to a base class causes fatal when child classes are not updated accordingly.

Compile Error: Type of MyErrorHandler::$errorRenderers must be array (as in class Slim\Handlers\ErrorHandler)

Should this not have been a major version bump? @l0gicgate
Maybe not - a protected property is not public interface. I just wanted to point out however, it might have been communicated in the release notes or similar, since I see many other places the types have been added in 4.11 release. But don't get me wrong - I'm glad you are adding the types.

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.

None yet

5 participants