[4.1.0] UuidV1::fromString(...) is not instance of UuidV1 anymore - breaking change?

[jacekkarczmarczyk]

Describe the bug

Using the following code I was able to check whether $uuidString is a v1 UUID,

$validV1 = UuidV1::fromString('08a527be-8e7a-11e9-bc42-526af7764f64') instanceof UuidV1;

Now it returns false, so:

1. maybe it wasn't the best option to check it - how would you do it in 4.1?
2. regardless whether it's best or not solution - it looks like a breaking change which shouldn't happen in minor release

[ramsey]

Thanks for the report, @jacekkarczmarczyk!

You're correct. 4.1.0 should not include breaking changes. However, it does include performance improvements that might have some unexpected consequences, like what you're seeing here.

@Ocramius, thoughts?

[Ocramius]

As I mentioned in the original PR, the signature of Uuid::fromString() only declares that a UuidInterface will be returned, not which string.

No BC break, since no signatures changed.

I can run roave/backward-compatibility-check on the patch, if you need more detail, but I've really been careful to respect existing signatures.

[Ocramius]

As for how to do it in 4.1.0 or newer, I endorse using the FieldsInterface (can be fetched from the Uuid instance)

[jacekkarczmarczyk]

As for how to do it in 4.1.0 or newer, I endorse using the FieldsInterface (can be fetched from the Uuid instance)

Can you elaborate? FieldsInterface has only getBytes() method that returns a string, I don't see how I would check the version of the UUID string. I mean

Uuid::fromString('...')->getFields()

actually returns instance of Fields class but one should assume that only getBytes methods is guaranteed to be implemented on the returned object

EDIT:
Unless you mean something like this:

$x = new \Ramsey\Uuid\Rfc4122\Fields(Uuid::fromString('38e39072-3c2f-4f3a-a8ab-2f6be1ea3136')->getBytes());
print $x->getVersion();

[Ocramius]

Indeed, annoying: FieldsInterface seems to be completely useless.

uuid/src/Fields/FieldsInterface.php

Lines 26 to 32 in 69b3eb8

	interface FieldsInterface extends Serializable
	{
	/**
	* Returns the bytes that comprise the fields
	*/
	public function getBytes(): string;
	}

I think what you posted above makes sense, even if it is indeed a very ugly API:

$x = new \Ramsey\Uuid\Rfc4122\Fields(Uuid::fromString('38e39072-3c2f-4f3a-a8ab-2f6be1ea3136')->getBytes());
print $x->getVersion();

[ramsey]

FieldsInterface is for introducing ULIDs and other options that don't conform to RFC 4122.

[ramsey]

Since the minimum version of the library is still on PHP 7.2, I'm unable to update the return values of the Rfc4122 classes to use the more-specific Rfc4122\FieldsInterface.

[Ocramius]

@ramsey yeah, I think there needs to be a bit of a harder split between the userland API (UuidInterface consumers) and advanced users (people that introspect the UUIDs post-instantiation)

[537mfb]

Since last update to ramsey/uuid I've been having an oddissue using ramsey/uuid-doctrine in symfony

My id is setup as

/**
 * @ORM\Id()
 * @ORM\Column(type="uuid_binary_ordered_time", unique=true)
 * @ORM\GeneratedValue(strategy="CUSTOM")
 * @ORM\CustomIdGenerator(class="Ramsey\Uuid\Doctrine\UuidOrderedTimeGenerator")
 */
private $id;

Since the last update the uuids generated have become version 2 - but then it validates agaisnt version 1 when i read them and fails

Is this related or should i open a new issue?

[Ocramius]

@537mfb I'd say you need a test case and a new issue for it :)

[fmonts]

This is a breaking change also if you have a Doctrine entity like this:

/**
 * @ORM\Column(type="uuid_binary", nullable=true)
 */
private ?Uuid $clickid;

Now you get a

Typed property App\Entity\...::$clickid must be an instance of Ramsey\Uuid\Uuid or null, Ramsey\Uuid\Lazy\LazyUuidFromString used

Should we change to Ramsey\Uuid\UuidInterface?

[Ocramius]

Should we change to Ramsey\Uuid\UuidInterface?

Yup, also there, it is documented as UuidInterface:

• https://github.com/ramsey/uuid-doctrine/blob/15961598d1fb440444c345b41aec9b1e09ab91b7/README.md#usage
• https://github.com/ramsey/uuid-doctrine/blob/15961598d1fb440444c345b41aec9b1e09ab91b7/src/UuidBinaryType.php#L57

[marartner]

Related to OP's issue, I'd like to point out that the page for upgrading version 3 to 4 claims that Uuid::fromString returns one of the more specific types, which it doesn't. This was what I was relying on and why I chose the same approach as OP - which ofc now backfired. Might wanna add a correction to that page if this is gonna stay this way, before somebody else stumbles over this too.

[ramsey]

The documentation was correct before version 4.1.

I need to decide how to address this.

The original goal was that fromString() would convert the string UUID to whatever subtype it represents. Those subtypes should all implement UuidInterface, so while the return type is correct, I can't reliably guarantee that one of the subtypes will be returned, so that can't be part of the contract. 😢

I implemented a lot of abstractions in version 4 that should help me implement some other identifier types in the (near) future, but these abstractions caused tremendous overhead in the library. @Ocramius provided some much-needed relief from the sluggishness of the implementation, but that broke users' expectations (though it didn't break the interface).

In some ways, my rush to get 4.0 out was a disaster, but I would never have caught the performance issues on my own, so releasing it as GA was the best approach to get the community to find them. Now that we know about the issues and have benchmarking scripts in place (thanks again, @Ocramius!), I can make some revisions that will hopefully help accomplish my goal of improved abstractions while improving performance.

[Ocramius]

@ramsey as per discussions in #324, if "advanced users" (people that care about the UUID details, rather than just passing around a UuidInterface) require it, there should probably be a separate API for them, for example UuidFactory::fromString() being called directly or such.

Considering that ramsey/uuid:4.0.0 was released 6 months ago, the amount of consumers broken (yes, some people's code is broken, but I stand my ground on stating that it was broken upfront) is not that massive, so we can still add a migration path in 4.2.0 that adds new "cpu-intensive" ::fromString() and ::fromBytes() logic that operated like before, but on a dedicated endpoint, and users affected by this problem would then need to mitigate the change in their code.

Using UuidFactory::fromString() directly could be put in the docs as being that particular endpoint, but there's no reliable way to guarantee that long-term we won't swap out the concrete UUID implementation (nor for FieldsInterface).

That is a design flaw highlighted by the various @psalm-suppress UndefinedInterfaceMethod:

uuid/src/Lazy/LazyUuidFromString.php

Line 288 in e4562b0

* @psalm-suppress UndefinedInterfaceMethod

I think we'll need a factory with a concrete union type, instead of UuidInterface being returned.

[jacekkarczmarczyk]

Why is even UuidV1::fromString('...') returning anything if the argument is not a valid UUID1? Shouldn't it throw?

[ramsey]

fromString() is not part of UuidInterface, so it's not supported on UuidV1. The fact that it's appearing on UuidV1 is due to inheritance.

[danfoley]

This seems related to me:

class Test
{
    public function __construct(Ramsey\Uuid\Rfc4122\UuidInterface $userId)
    {
    }
}

new Test(Ramsey\Uuid\Uuid::uuid4());

throws:
TypeError: Argument 1 passed to Test::__construct() must be an instance of Ramsey\Uuid\Rfc4122\UuidInterface, instance of Ramsey\Uuid\Lazy\LazyUuidFromString given

How can I use the Rfc4122 UuidInterface if the uuid functions don't return instances of it? I'd use the Ramsey\Uuid\UuidInterface instead, but then I have no way to check the version of them.

[Ocramius]

This should do:

uuid/src/UuidFactory.php

Lines 305 to 313 in fe665a0

	/**
	* @psalm-pure
	*/
	public function fromString(string $uuid): UuidInterface
	{
	$uuid = strtolower($uuid);
	return $this->codec->decode($uuid);
	}