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

Removed strict types from the code #22

Closed
wants to merge 1 commit into from
Closed

Conversation

bgorski
Copy link

@bgorski bgorski commented Jun 25, 2021

Signed-off-by: Bartosz Gorski [email protected]

Q A
Documentation no
Bugfix no
BC Break yes
New Feature no
RFC no
QA no

Description

The introduction of declare(strict_types=1); made the change backwards incompatible with systems using laminas/laminas-escaper as a dependency in a non-strict way. For example Magento: magento/magento2#33346
Don't get me wrong - it's an absolutely great idea to use strict types everywhere. But because of backwards incompatibility reasons, it shouldn't be enforced in a patch version increase (2.7.0 -> 2.7.1).

  • Are you fixing a BC Break?
    • How do you reproduce it?
      • This is an example of a non-strict usage fixed to a strict one: https://github.com/magento/magento2/pull/33353/files. To reproduce it, use the escapeUrl with a null or an int for example (which doesn't make sense when you have full awareness of the system, but may happen in huge applications)
    • What was the previous behavior?
      • the input value of most Escaper class methods would be cast to string at some point of the execution of the logic
    • What is the current behavior?
      • an error is thrown when non-string value is provided

@Ocramius
Copy link
Member

Closing: duplicate of #20 - see discussion there.

@Ocramius Ocramius closed this Jun 26, 2021
@Ocramius Ocramius self-assigned this Jun 26, 2021
@Ocramius Ocramius added BC Break Bug Something isn't working Duplicate This issue or pull request already exists Invalid This doesn't seem right labels Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break Bug Something isn't working Duplicate This issue or pull request already exists Invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants