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

Stricter input/output type declaration, matching original type signatures from 2.0.0-beta5 release #23

Merged
merged 4 commits into from
Jun 26, 2021

Conversation

carnage
Copy link
Contributor

@carnage carnage commented Jun 25, 2021

Signed-off-by: Carnage t.carnage@gmail.com

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

Description

The introduction of strict types in the file heading is propagated to calling code as the required types are missing from the method signatures. This PR adds the types and resolves the BC break.

While adding types to the methods could be considered a BC break in itself, I cannot conceive of any sensible use case that this change would impact.

Signed-off-by: Carnage <t.carnage@gmail.com>
@Ocramius Ocramius changed the base branch from 2.7.x to 2.8.x June 26, 2021 14:04
@Ocramius Ocramius changed the title Fix BC break by introduction of strict types Stricter input/output type declaration Jun 26, 2021
@Ocramius Ocramius self-assigned this Jun 26, 2021
@Ocramius Ocramius added this to the 2.8.0 milestone Jun 26, 2021
@Ocramius
Copy link
Member

Marked for 2.8.0.

Effectively, this is an improvement that mitigates the problems found in #20, and if call-site consumers of the code use declare(strict_types=0) (equivalent to not declaring strict types), this fixes some issues for them.

In addition to that, I decided to not mark this as BC Break, since:

@Ocramius
Copy link
Member

Ocramius commented Jun 26, 2021

I'll merge this manually after having applied some cleanup operations (CS/QA checks, mostly)

Since the constructor of `Escaper` now asserts the input to be of type `string|null`,
we no longer need to assert by hand on mis-behavior, and therefore can remove an
exception scenario, as well as the associated test case.
Since we now use native parameter type declarations, we no longer need
to have `@param string $param` declarations.

Also, `is_string()` and `gettype()` are no longer in use (were used when
throwing an exception based on invalid input).

Note that `@return string` is preserved: we cannot introduce native return
type declarations, as that would lead to downstream crashes in inheritance
scenarios, when the consumer didn't declare `: string` return types themselves
too.
@Ocramius Ocramius changed the title Stricter input/output type declaration Stricter input/output type declaration, matching original type signatures from 2.0.0-beta5 release Jun 26, 2021
@Ocramius Ocramius merged commit 2d6dce9 into laminas:2.8.x Jun 26, 2021
@Ocramius
Copy link
Member

Thanks @carnage!

@carnage carnage deleted the fix-strict-type-bc branch June 26, 2021 16:16
@hostep
Copy link

hostep commented Jun 27, 2021

Thanks @Ocramius & @carnage: I can confirm that version 2.8.0 fixes the problem in Magento.
Thanks for the quick action! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants