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 optional type $description in ActionError can not be set to null #290

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

Dmitrev
Copy link
Contributor

@Dmitrev Dmitrev commented Jul 15, 2022

Please see this issue for context:
#289

@l0gicgate
Copy link
Member

This would be a breaking change. Instead, we should correct the type to be properly nullable.

@Dmitrev
Copy link
Contributor Author

Dmitrev commented Jul 26, 2022

@l0gicgate sorry for the late response, how would it be a breaking change?

Setting the value to null would just cause it to crash, feel free to try it! :D

@l0gicgate
Copy link
Member

@Dmitrev you just answered your own question. Changing a nullable constructor parameter to non nullable is a breaking change. That means it’s not backward compatible.

I know that this is a skeleton repo and it wouldn’t break things downstream but that’s not the point.

Also, this parameter should be nullable as you may not always pass an error message.

@Dmitrev
Copy link
Contributor Author

Dmitrev commented Jul 28, 2022

@Dmitrev you just answered your own question. Changing a nullable constructor parameter to non nullable is a breaking change. That means it’s not backward compatible.

I know that this is a skeleton repo and it wouldn’t break things downstream but that’s not the point.

Also, this parameter should be nullable as you may not always pass an error message.

@l0gicgate Sorry I have to disagree with you here. It is already broken as is. Passing null would cause it to crash. If anyone passed null in the constructor in their code base, they would have had to change this.

See example:
https://onlinephp.io/c/507ad

Fatal error: Uncaught TypeError: Cannot assign null to property ActionError::$description of type string in /home/user/scripts/code.php:22
Stack trace:
#0 /home/user/scripts/code.php(58): ActionError->__construct('myType', NULL)
#1 {main}
  thrown in /home/user/scripts/code.php on line 22

Same with the setter

https://onlinephp.io/c/4e553

Fatal error: Uncaught TypeError: Cannot assign null to property ActionError::$description of type string in /home/user/scripts/code.php:43
Stack trace:
#0 /home/user/scripts/code.php(59): ActionError->setDescription()
#1 {main}
  thrown in /home/user/scripts/code.php on line 43

It cannot be a breaking change, if it never worked to begin with...

I am happy for it to be nullable, can change that

@l0gicgate
Copy link
Member

@Dmitrev you’re right it’s already broken. The constructor is fine, the property itself should be nullable though as I mentioned.

@Dmitrev
Copy link
Contributor Author

Dmitrev commented Jul 29, 2022

@l0gicgate updated it with your suggestion

@Dmitrev Dmitrev changed the title Remove optional type description ActionError Fix optional type $description in ActionError can not be set to null Jul 29, 2022
@l0gicgate
Copy link
Member

Thank you for your contribution @Dmitrev

@l0gicgate l0gicgate merged commit c8bc550 into slimphp:master Aug 1, 2022
@Dmitrev Dmitrev deleted the remove-optional-types-actionerror branch August 9, 2022 17:20
@akrabat akrabat added this to the 4.5.0 milestone Nov 2, 2022
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.

4 participants