If we suddenly start to throw a different exception, wouldn't this break existing code that catches the "old" exception?

Hi @derrabus ,

If we suddenly start to throw a different exception, wouldn't this break existing code that catches the "old" exception?

That's a great question.

No, it wouldn't break, since it has never been released.

That's why I'm opening this PR now, so that these two exceptions won't create backward compatibility promise starting from 7.3 and onward.

Yes, it is not released yet

it's possible to determine whence the exception originated from by checking its getFile, or checking the trace.

This doesn't look like a good practice to me, so this sounds like a counter argument.

What benefit do you expect from this PR over the current situation?

Coincidentally, php-src is working on php/policies#17, that might give some food for thoughts on the topic.

Specifically, I want to unify what exceptions are thrown form uid-classes.

What I mean is following:

• If you call Uuid::fromBase32(), currently you might get InvalidArgumentException, or InvalidUuidException;
• If you call Ulid::fromBase32(), currently you might get InvalidArgumentException, or InvalidUlidException;

I want to unify it all so that whether you call Uuid::fromBase32(), or Ulid::fromBase32(), you'll get the same InvalidUidException that is related to AbstractUid rather than to any concrete implementation.

Benefit is that InvalidUidException is conceptually self-evident - it is thrown when creating uid.

I am not in favor of this change. If one wants to only care for one exception than for either InvalidUuidException or InvalidUlidException that's already possible by catching InvalidArgumentException instead. For anyone wanting to distinguish UUID and ULID having distinct exceptions is better.

One can not distinguish between Uuid::fromBase32() and Ulid::fromBase32() (and fromBinary, fromBase58, fromRfc4122 as well) since these might fail due to the length check is in AbstractUid:

public static function fromBase32(string $uid): static
{
    if (26 !== \strlen($uid)) {
        throw new InvalidArgumentException($uid, 'Invalid base-32 uid provided.');
    }

    return static::fromString($uid);
}

So then it's not possible to distinguish anyway.

I am not in favor of this change.

As far as I remember, you were not in favor of the exceptions being introduced either.

Also, I remember @nicolas-grekas was for the idea to have one single InvalidUidException, while I stated that it's better to have them separate.

Now, I revised the code and see that it's better to have the unified interface so that make fromBinary, fromBase58, fromBase32, fromRfc4122 (actually any method that creates uid) throw it. It's totally clear from UI standpoint that InvalidUidException is thrown due to the creation

Hi guys, I would like to move on if you don't mind

I'm AFK until next week, I'll come back here ASAP.

Sure, have a nice time

Just want to make sure this doesn't get off to 7.3 release

It's a small change, I think it mustn't need much work

@nicolas-grekas , @fabpot , just a quick check-in, since I don't want to introduce BC promise for 7.3, is it all right right?

InvalidUidException must be kept, as it keeps the meaning of uuid creation.

@nicolas-grekas , hi again.

Sorry for long waiting. So about this, it's more easily grasped by short name

@nicolas-grekas , I'd rather keep InvalidUidException, as when caught / logged, it is obvious without looking at full namespace

My concern is about keeping both InvalidArgumentException and InvalidUidException : I had a look at where each is thrown and I'm not sure the difference makes sense.

I agree with Nicolas. Right now we only throw those exception when UIDs are invalid thus both exceptions carry the same semantics.

As far as I know, there are some other places where InvalidArgumentException is thrown

As far as I know, there are some other places where InvalidArgumentException is thrown

Yes, and my point is that throwing a different type of exception doesn't make sense to me (InvalidArgumentException vs InvalidUidException)

Try to look at it from the client code perspective

• if anybody wants to catch the exception, InvalidUidException is much more friendly than InvalidArgumentException, as understanding of the code doesn't require looking up to the imports section
• there are quite a few other invalid argument exceptions, and can be mis-imported
• if other exceptions imported at the same class, importing new would require using import aliases
• looking at exception in logs is similar - when we glance, we only see the last part, and it may be confusing for a while to know that this exception is that uuid is invalid

You're missing the point I'm making. Let's forget about the name, that's not what I'm talking about.

Sorry, then I didn't get your point

Let's say we keep two types as currently proposed in this PR: InvalidArgumentException and InvalidUidException.
Having two types means there should be a specific use case for each of them.
But when I look at the code, I don't see any good criteria to decide between the two: we could always throw eg InvalidUidException and that wouldn't downgrade anything for end users.

We could've kept InvalidUidException if InvalidArgumentException was not used

@rela589n it would be super helpful if you could open the code to understand why I'm having this opinion about the type hierarchy.
About the name: if only one exception is needed (which is what I'm thinking), then it should be InvalidArgumentException. I read your arguments in favor of InvalidUidException but consistency with naming in all other components is more important IMHO.

Usages of InvalidArgumentException:

UuidV6::toV7() (frankly speaking I don't know how it extracts time and why this can go wrong; in my opinion LogicException should've been used instead)

$uuid = $this->uid;
$time = BinaryUtil::hexToNumericString('0'.substr($uuid, 0, 8).substr($uuid, 9, 4).substr($uuid, 15, 3));
if ('-' === $time[0]) {
    throw new InvalidArgumentException('Cannot convert UUID to v7: its timestamp is before the Unix epoch.');
}

UuidV7::generate(?\DateTimeInterface $time = null) - throws if provided time is invalid. Notice - if provided time is invalid, not provided uuid

if (null === $mtime = $time) {
    $time = microtime(false);
    $time = substr($time, 11).substr($time, 2, 3);
} elseif (0 > $time = $time->format('Uv')) {
    throw new InvalidArgumentException('The timestamp must be positive.');
}

Ulid::generate(?\DateTimeInterface $time = null) - throws exception if provided time is invalid

if (null === $mtime = $time) {
    $time = microtime(false);
    $time = substr($time, 11).substr($time, 2, 3);
} elseif (0 > $time = $time->format('Uv')) {
    throw new InvalidArgumentException('The timestamp must be positive.');
}

BinaryUtil::dateTimeToHex(\DateTimeInterface $time) - throws exception if provided time is invalid (used only in UuidV1::generate())

if (-self::TIME_OFFSET_INT > $time = (int) $time->format('Uu0')) {
    throw new InvalidArgumentException('The given UUID date cannot be earlier than 1582-10-15.');
}

So therefore, all these usages are related to uuid generation, which is different from uuid reconstruction.

Besides that, as other components define InvalidArgumentException as is, and it is used in the respective places, it's worth having it here as well.

There are two kinds of this exception - one related to Uuid intended to be created from the value, and the other's related to generating Uuid.

@nicolas-grekas , please let me know if this makes sense to you

AFAIK the rare cases a component contains both an InvalidArgumentException and exception(s) that extend it... is done to specify the very nature of the exception, not provide information about the argument itself.

Exemples:

Mailer

• Symfony\Component\Mailer\Exception\IncompleteDsnException

DependencyInjection

• Symfony\Component\DependencyInjection\Exception\InvalidParameterTypeException, EnvParameterException, EnvNotFoundException, etc

OptionsResolver

• Symfony\Component\OptionsResolver\Exception\InvalidOptionsException (in opposition to MissingOptionsException and UndefinedOptionsException)

etc etc..

So.. imho any class extending InvalidArgumentException should define the reason of the problem, not its type.. and i would "vote" to only keep InvalidArgumentException for now.

I submitted #60530 instead. It doesn't make sense to me to have two kind of exceptions for that. About the name, I already explained the rationale to go with InvalidArgumentException: your arguments apply not only to Uid but to all other components. This means for consistency we'd want to change the name in all components. That's not going to happen as the migration cost would be too high compared to the alleged benefits.

Thanks for submitting and for the discussion!

class extending InvalidArgumentException should define the reason of the problem, not its type

@smnandre , class inherently represents the type

About the name, ... not only to Uid but to all other components

@nicolas-grekas , probably you misunderstood me, since as I've said in the previous message insofar as other components define InvalidArgumentException, it's worth having it here as well. I have no intention to make drastic changes in exception usage.

I just said that it's good to have dedicated InvalidUidException, representing type for full set of reasons about uid value.