Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 2, 2024

@staabm staabm marked this pull request as ready for review August 2, 2024 05:38
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 164691d into phpstan:1.11.x Aug 2, 2024
449 of 457 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@ondrejmirtes
Copy link
Member

Some new error appeared based on this, can you look if it's valid or not? https://github.com/phpstan/phpstan/actions/runs/10226167599/job/28296101528

@staabm
Copy link
Contributor Author

staabm commented Aug 3, 2024

Some new error appeared based on this, can you look if it's valid or not? phpstan/phpstan/actions/runs/10226167599/job/28296101528

thats a interessting case: https://github.com/PrestaShop/PrestaShop/blob/ca9d81aae0bb17f3e767bb5d835348ed144a3ab5/src/Adapter/Assets/AssetUrlGeneratorTrait.php#L57

@Seldaek do you agree that for proper escaping the PrestaShop code would need a tripe backslash like in https://3v4l.org/8fsWi ?

another example - which IMO also proves the regex in PrestShop is missing a 3rd backslash - is https://3v4l.org/Td9vj

@Seldaek
Copy link
Contributor

Seldaek commented Aug 3, 2024

Yeah that's a case of needing 4 backslashes imo.. You really want one in the pattern so you have to escape it as 2 and then each of those again escaped for the php string. Technically 3 works the same i suppose as long as it's not double quoted strings and depending what follows the 3rd.

@ondrejmirtes
Copy link
Member

FYI this is currently blocking me from release, this bug is still present and generating errors in Prestashop.

@staabm
Copy link
Contributor Author

staabm commented Aug 5, 2024

As I understood it, its a bug in prestashop because they wrongly escape.

@ondrejmirtes
Copy link
Member

OK, thanks!

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