function escapeUrl accept only string

ikac5 commented 3 years ago

Bug Report

Can't view any product in magento in admin panel

Summary

Current behavior

TypeError: rawurlencode() expects parameter 1 to be string, int given in /var/www/vhosts/wag.ls/html/vendor/laminas/laminas-escaper/src/Escaper.php:246

How to reproduce

Expected behavior

maglnet commented 3 years ago

It seems like a similar problem happens for escapeHtmlAttr, escapeJs and escapeCss after declare(strict_types=1); was introduced in 2.7.1 yesterday.

Ocramius commented 3 years ago

Seems about right: they always expected a string as input, and it should be a string as input.

The bug here is on the call side, not in the escaper.

On Fri, Jun 25, 2021, 12:14 Matthias Glaub @.***> wrote:

It seems like the same happens for escapeHtmlAttr, escapeJs and escapeCss after declare(strict_types=1); was introduced in 2.7.1 yesterday.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/laminas/laminas-escaper/issues/20#issuecomment-868394175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEE3MNHONJMTMI3A2KDTURJIZANCNFSM47JSTDAQ .

oburk commented 3 years ago

I can confirm this bug. I got the following PHP warnings after update from laminas/laminas-escaper version 2.7.0 to 2.7.1

[25-Jun-2021 11:49:32 Europe/Berlin] TypeError: preg_match() expects parameter 2 to be string, int given in /backend/vendor/laminas/laminas-escaper/src/Escaper.php:403
#0 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(403): preg_match('/^./su', 1045)
#1 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(371): Laminas\Escaper\Escaper->isUtf8(1045)
#2 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(204): Laminas\Escaper\Escaper->toUtf8(1045)
#3 /backend/lib/Base/Helper/String.php(2202): Laminas\Escaper\Escaper->escapeHtmlAttr(1045)

Ocramius commented 3 years ago

Again: not a bug, but expected behaviour.

string has always been the documented input type, so please do adjust to it

On Fri, Jun 25, 2021, 12:23 Oliver Burk @.***> wrote:

I can confirm this bug. I got the following PHP warnings after update from laminas/laminas-escaper version 2.7.0 to 2.7.1

[25-Jun-2021 11:49:32 Europe/Berlin] TypeError: preg_match() expects parameter 2 to be string, int given in /backend/vendor/laminas/laminas-escaper/src/Escaper.php:403

0 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(403): preg_match('/^./su', 1045)

1 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(371): Laminas\Escaper\Escaper->isUtf8(1045)

2 /backend/vendor/laminas/laminas-escaper/src/Escaper.php(204): Laminas\Escaper\Escaper->toUtf8(1045)

3 /backend/lib/Base/Helper/String.php(2202): Laminas\Escaper\Escaper->escapeHtmlAttr(1045)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/laminas/laminas-escaper/issues/20#issuecomment-868399136, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEFLNERO64I2GP5RFLDTURKJ3ANCNFSM47JSTDAQ .

oburk commented 3 years ago

I agree it is not a technical bug, but it is not common to change function behavior inside a patch release.

maglnet commented 3 years ago

Well, yes. Nevertheless looking into the code, there was additional handling for digits as well (the ctype_digit check). https://github.com/laminas/laminas-escaper/blob/77c248d9483cfd6e72554cc85c7c9f1caccf8be3/src/Escaper.php#L205-L207

@Ocramius Would there be chances to allow getting integers to work again by providing a MR, or wouldn't you accept this?

Ocramius commented 3 years ago

Agree on the patch release change.

As for accepting integers: no, that is categorically excluded by the API definition of these methods, since a decade.

On Fri, Jun 25, 2021, 12:56 Oliver Burk @.***> wrote:

I agree it is not a technical bug, but it is not common to change function behavior inside a patch release.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/laminas/laminas-escaper/issues/20#issuecomment-868416101, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEF5UMZCS6BTTKEHFFTTUROGTANCNFSM47JSTDAQ .

maglnet commented 3 years ago

Ok, I'm fine with it 👍 but not happy :-D

gntlby commented 3 years ago

I can't see the products on both the frontend and the admin panel.

TypeError: rawurlencode() expects parameter 1 to be string, bool given in /vendor/laminas/laminas-escaper/src/Escaper.php:246

Ocramius commented 3 years ago

Closing here: please refer to your stack trace, and find the first occurrence of a call to the Escaper that comes from a component that is not passing it a string.

The API signature states that only strings should be passed in.

This string signature exists in laminas/laminas-escaper since 2.0.0-beta5 (its initial introduction): https://github.com/zendframework/zendframework/blob/18c8e223f070deb07c17543ed938b54542aa0ed8/library/Zend/Escaper/Escaper.php#L196-L199

That is from 2012-07-04, which is 9 years ago.

no release allowed for int to be explicitly passed in.

Blaimi commented 3 years ago

issued in magento as https://github.com/magento/magento2/issues/33346 with a fix in PR https://github.com/magento/magento2/pull/33353.

hostep commented 3 years ago

@Ocramius: thanks for linking to this issue, I didn't see it before because it was already closed.

Should we at least keep it open and have a further discussion about this?

Would be great if we can remove the declare(strict_types=1); additions of version 2.7.1 and release a new 2.7. 2 version. You can add declare(strict_types=1); later again in version 3.0.0

This package is part of a framework, you should try to do your best to not cause backwards compatible breaking changes if possible. And yes having declare(strict_types=1); is nice, but not at the cost of the users of your framework to cause them headaches in minor or patch version upgrades. Even if those users of your framework are calling the methods with incorrectly typed parameters.

/cc @weierophinney, @ghostwriter

weierophinney commented 3 years ago

@hostep see https://github.com/laminas/laminas-escaper/issues/21#issuecomment-868542534

Ocramius commented 3 years ago

This package is part of a framework, you should try to do your best to not cause backwards compatible breaking changes if possible.

This package was never compatible with non-string input: that is an accident, not a BC compliance surface.

BC compliance is all fine (I literally made a whole talk about it, and wrote tooling to check it), but it is limited to what is considered acceptable usage.

pcGarethJames commented 3 years ago

This package was never compatible with non-string input: that is an accident, not a BC compliance surface.

Accident or not it was still compatible with non-string values, and was also not enforcing types either so surely any change to rectify that accident would be a BC breaking change, regardless of any amount documentation/history to that function?

I agree these things need to be enforced, and that Magento needs to sort its side out, but I agree with @hostep that this should be a major change, not a patch.

WillyBaldy commented 3 years ago

Accident or not it was still compatible with non-string values, and was also not enforcing types either so surely any change to rectify that accident would be a BC breaking change, regardless of any amount documentation/history to that function?

Totally agree, I really think that adding declare(strict_types=1); in source code for a minor release is highly risky, there's so much side effects possible especially in a framework. The argument that the signatures were documented in such a way since the beginning of time is great in theory, but tell that to my test units :-D

Ocramius commented 3 years ago

tell that to my test units :-D

You evidently have some properly invalid scenarios in there.

I'm closing + locking here - the chapter is closed, and specified types are not an opinion.

laminas / laminas-escaper