8000 [FrameworkBundle][Uid] UuidV1 case depends on `uuid` installed or not · Issue #57878 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle][Uid] UuidV1 case depends on uuid installed or not #57878

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.

Sign up for GitHub

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

Jump to bottom
Closed
smnandre opened this issue Jul 29, 2024 · 8 comments · Fixed by #57887
Closed

[FrameworkBundle][Uid] UuidV1 case depends on uuid installed or not #57878

smnandre opened this issue Jul 29, 2024 · 8 comments · Fixed by #57887
Labels
Bug FrameworkBundle Status: Needs Review Uid

Comments

@smnandre
Copy link
Member

Symfony version(s) affected

6.4, 7.0, 7.1 (?)

Description

I found a strange bug while running the FrameworkBundle tests locally

Symfony\Bundle\FrameworkBundle\Tests\Functional\UidTest::testArgumentValueResolverEnabled
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'0aa49ee6-4de7-11ef-9275-ca8c1eeac807'
+'0AA49EE6-4DE7-11EF-9275-CA8C1EEAC807'

// Symfony/Bundle/FrameworkBundle/Tests/Functional/UidTest.php

$client->request('GET', '/1/uuid-v1/'.$uuidV1 = new UuidV1());
$this->assertSame((string) $uuidV1, $client->getResponse()->getContent());

It took me some time to realize my local PHP still had the "uid" extension installed.

So the problem comes down to this in

// Symfony/Component/Uid/UuidV1.php

public function __construct(?string $uuid = null)
{
    if (null === $uuid) {
        $this->uid = uuid_create(static::TYPE);
    } else {
        parent::__construct($uuid, true);
    }
}

uuid_create from the Symfony polyfill only generates lowercase strings, but the uuid_create from the extension does not.

So when the ArgumentResolver works, as it set the $uuid arguments, the parent::__construct is called, where a strtolower is applied to $this-uid

How to reproduce

Install the uuid extension and run the FrameworkBundle functional tests

Possible Solution

Add a strtolower (or any optimized strtr) in the UuiV1 constructor ? (if so i'll open a PR)

Or do you think we need to ensure all uid values are stored internally in lowercase ?

...

Or this should not be considered a bug at all ?

Additional Context

No response
@smnandre smnandre added the Bug label Jul 29, 2024
@stof
Copy link
Member
stof commented Jul 30, 2024

Given that the predefined routing requirements for uuids enforce lowercase, I think we should enforce the usage of lowercase even when using the PHP extension (if we want to optimize things when using the polyfill which already guarantees lowercase, we might want to use \extension_loaded('uuid') to make the check conditional but optimized by OPCache)

8000

@nicolas-grekas
Copy link
Member

And the polyfill should behave exactly like the extension.

@smnandre
Copy link
Member Author
smnandre commented Jul 30, 2024 •
edited
Loading

@stof @nicolas-grekas thanks for the quick answers!

So to confirm, next actions would be:

Symfony\Component\Uid\UuidV1

  • add a strtolower/strtr in the constructor (when $uuid is null) to enforce lowercase

Symfony\Polyfill\Uuid\Uuid

  • make the uuid_create returns uppercase string (to mimic the extension)

@stof there is alreay this check in the bootstrap.php file of the polyfill

Do you suggest we add it somewhere in the component code ?

@smnandre
Copy link
Member Author

Is strtr still something we use ?

Looks to me it's always slower than strtolower in recent PHP versions: https://3v4l.org/qbdah#v8.3.9

@nicolas-grekas
Copy link
Member

strtolower has been optimized in recent versions of PHP indeed

@derrabus
Copy link
Member

And the polyfill should behave exactly like the extension.

I recall that I've had that problem with the different casing on different machines a couple of years ago. I might be utterly mistaken, but I think I came to the conclusion that the behavior of the extension with regards to UUID casing depended on the libraries the extension was compiled against.

@smnandre smnandre mentioned this issue Jul 30, 2024
Merged
@smnandre
Copy link
Member Author

the behavior of the extension with regards to UUID casing depended on the libraries the extension was compiled against.

So... do you think we need to change the polyfill ?

@derrabus
Copy link
Member

No because the polyfill already reflects a correct behavior.

chalasr added a commit that referenced this issue Jul 31, 2024
@chalasr
bug #57887 [Uid] Ensure UuidV1 is created in lowercase (smnandre)
54641cb 
This PR was submitted for the 7.2 branch but it was merged into the 5.4 branch instead.

Discussion
----------

[Uid] Ensure UuidV1 is created in lowercase

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #57878
| License       | MIT

Ensure `$this->uid` is in lowercase in UuidV1  (see #57878  for context)

(strtolower is already called in the parent::__construct... but not when $args is null. And the uuid extension generates uppercase values --at least sometimes--).

Not sure if should be considered a "bug" / which version to flag ?

Commits
-------

9abfd25 [Uid] Ensure UuidV1 is created in lowercase
@chalasr chalasr closed this as completed Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review Uid
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Uid] Ensure UuidV1 is created in lowercase smnandre/symfony
6 participants
@nicolas-grekas @stof @smnandre @derrabus @chalasr @carsonbot
0