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 onphp/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 callUuid::fromBase32(), currently you might getInvalidArgumentException, orInvalidUuidException;
• If you callUlid::fromBase32(), currently you might getInvalidArgumentException, orInvalidUlidException;

I want to unify it all so that whether you callUuid::fromBase32(), orUlid::fromBase32(), you'll get the sameInvalidUidException that is related toAbstractUid rather than to any concrete implementation.

Benefit is thatInvalidUidException 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 eitherInvalidUuidException orInvalidUlidException that's already possible by catchingInvalidArgumentException instead. For anyone wanting to distinguish UUID and ULID having distinct exceptions is better.

One can not distinguish betweenUuid::fromBase32() andUlid::fromBase32() (andfromBinary,fromBase58,fromRfc4122 as well) since these might fail due to the length check is inAbstractUid:

publicstaticfunctionfromBase32(string$uid):static{if (26 !==\strlen($uid)) {thrownewInvalidArgumentException($uid,'Invalid base-32 uid provided.');    }returnstatic::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 haveone 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 makefromBinary,fromBase58,fromBase32,fromRfc4122 (actually any method that creates uid) throw it. It's totally clear from UI standpoint thatInvalidUidException 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 keepInvalidUidException, 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 whereInvalidArgumentException 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 thanInvalidArgumentException, 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 keptInvalidUidException ifInvalidArgumentException 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 beInvalidArgumentException. I read your arguments in favor ofInvalidUidException but consistency with naming in all other components is more important IMHO.

Usages ofInvalidArgumentException:

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]) {thrownewInvalidArgumentException('Cannot convert UUID to v7: its timestamp is before the Unix epoch.');}

UuidV7::generate(?\DateTimeInterface $time = null) - throws if provided time is invalid. Notice - ifprovided 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')) {thrownewInvalidArgumentException('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')) {thrownewInvalidArgumentException('The timestamp must be positive.');}

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

if (-self::TIME_OFFSET_INT >$time = (int)$time->format('Uu0')) {thrownewInvalidArgumentException('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 defineInvalidArgumentException 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 anInvalidArgumentExceptionand exception(s) that extend it... is done to specify the verynature 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 toMissingOptionsException andUndefinedOptionsException)

etc etc..

So.. imho any class extendingInvalidArgumentException should define thereason of the problem, not its type.. and i would "vote" to only keepInvalidArgumentException 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 dedicatedInvalidUidException, representing type for fullset of reasons about uid value.