function escapeUrl accept only string

[ikac5]

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]

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]

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 <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEE3MNHONJMTMI3A2KDTURJIZANCNFSM47JSTDAQ> .

[oburk]

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]

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 <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEFLNERO64I2GP5RFLDTURKJ3ANCNFSM47JSTDAQ> .

[oburk]

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

[maglnet]

Well, yes.
Nevertheless looking into the code, there was additional handling for digits as well (the ctype_digit check).

laminas-escaper/src/Escaper.php

Lines 205 to 207 in 77c248d

	if ($string === '' || ctype_digit($string)) {
	return $string;
	}

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

[Ocramius]

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 <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEF5UMZCS6BTTKEHFFTTUROGTANCNFSM47JSTDAQ> .

[maglnet]

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

[gntlby]

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]

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]

issued in magento as magento/magento2#33346 with a fix in PR magento/magento2#33353.

[hostep]

@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]

@hostep see #21 (comment)

[Ocramius]

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]

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]

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]

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.