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

Fix PSR16 compatibility by sanitizing Colon symbol #447

Closed
wants to merge 12 commits into from

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented May 3, 2024

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented May 3, 2024

@dbu I'm not sure how we can keep the functional test for the CacheClient here correctly as the dependency graph seems hard to control what is compatible with the specific symfony cache.

@dbu dbu changed the title Fix PSR16 compatibility by sanitze Colon symbol Fix PSR16 compatibility by sanitizing Colon symbol May 4, 2024
@dbu
Copy link
Member

dbu commented May 4, 2024

thanks for this. kinda weird how many characters are not allowed in the key. psr-16 specifially forbids {}()/\@: and says at minimum A-Z, a-z, 0-9, _, and . must be supported. we should probably transliterate everything onto the minimal characeter set. we could use urlencode and then replace % with _. having an explicit list of allowed things is better than escaping specific things and then the next person comes with a node with 🛩️ in its name and some cache not happy with that.

RE simple-cache version: that is indeed unfortunate. it seems symfony 5 had psr-16 only as dev-dependency and thats the reason why composer does not know this won't work.

as this is the legacy 1.x branch, i propose we specifically do a composer require "psr/simple-cache:2.*" in the test-application.yaml file if php-version is equal to 8.0.

not super elegant, but its a bit of an edge case and not the reponsiblity of jackalope-doctrine-dbal .

want to do that?

@alexander-schranz
Copy link
Contributor Author

@dbu urlencode does not catch . so we need to replace atleast that character manually. I pushed a version and adopted the testcase.

@dbu
Copy link
Member

dbu commented May 6, 2024

@dbu urlencode does not catch . so we need to replace atleast that character manually. I pushed a version and adopted the testcase.

right. according to https://www.php.net/manual/en/function.urlencode.php that should be the only mismatch, right?

@alexander-schranz
Copy link
Contributor Author

@dbu I think so.

composer.json Show resolved Hide resolved
@dbu
Copy link
Member

dbu commented May 6, 2024

thanks a lot! squashed and added changelog entries in #448

@dbu dbu closed this May 6, 2024
@alexander-schranz alexander-schranz deleted the patch-2 branch May 6, 2024 15:16
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.

2 participants