[PSR-6] Concrete exception classes

[Bernhard Schussek]

Hi all,

Thanks for bringing PSR-6 to Review! I did a first review now. I think that the exceptions should be concrete classes and not interfaces. We did the same in PSR-3 and I think there's no need to have an exception interface.

Cheers,

Bernhard

--

Bernhard Schussek
Blog: http://webmozarts.com
Twitter: http://twitter.com/webmozart

[Larry Garfield]

On 8/3/15 3:55 AM, Bernhard Schussek wrote:
> Hi all,
>
> Thanks for bringing PSR-6 to Review! I did a first review now. I think
> that the exceptions should be concrete classes and not interfaces. We
> did the same in PSR-3 and I think there's no need to have an exception
> interface.
>
> Cheers,
> Bernhard

Hi Bernhard.

Why not an interface? Any exception thrown by a cache system should be
denoted as coming from a cache system (that's the goal), but it could
still legitimately be a \RuntimeException, and
\InvalidArgumentException, or something else to extend. Extending is
something that should be reserved for implementers close to the
situation, rather than the API design, I'd argue.

--
--Larry Garfield

[Robert Hafner]

I agree with Larry on this one (I know, shocking). It’s much better to allow people to extend the interfaces in my view.

I will say though that we should probably make the InvalidArgumentException implement the CacheException interface if we really want all of the exceptions thrown by the standard to be catchable in one shot.

Rob

> --
> You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
> To post to this group, send email to php...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/55BF9337.9060003%40garfieldtech.com.
> For more options, visit https://groups.google.com/d/optout.

[Larry Garfield]

/me dies of shock.

That does make sense. I think, technically, the prose text says that
needs to be done (ie, the exception you throw would need to implement
both interfaces), but that's not at all clear. I'd be in favor of
making the cache InvalidArgumentException extend CacheException in the code.

Beau, do you feel that's a Review-safe change? (I would argue yes,
given the above.)

--Larry Garfield

--
--Larry Garfield

[Beau Simensen]

On Monday, August 3, 2015 at 12:05:39 PM UTC-5, Larry Garfield wrote:

That does make sense.  I think, technically, the prose text says that
needs to be done (ie, the exception you throw would need to implement
both interfaces), but that's not at all clear.  I'd be in favor of
making the cache InvalidArgumentException extend CacheException in the code.

Beau, do you feel that's a Review-safe change?  (I would argue yes,
given the above.)

Since we are probably going back to draft anyway, the question of whether this is review safe or not is somewhat moot. I think that this change should be made, especially since you and Robert both agree.

As for my stance, I think it would indeed be nice if these  were proper exceptions but from what I can tell it would limit how implementations can generate and store exceptions. That said, we do do this in PSR-3 and many projects ship concrete exceptions despite the fact that adapter/implementations are expected to use them. If only PHP had a Throwable interface from which we could extend... :)

[Robert Hafner]

Why are we going back to draft? I thought we were making the one change and then returning to review. I think the exceptions are fine for now, and we as a group will most likely have to revisit exceptions for everything once "throwable" is the standard.

Rob

--

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/b4d4ce94-1613-4e15-8cfa-34a028b5732e%40googlegroups.com.

[Beau Simensen]

On Tuesday, August 18, 2015 at 8:09:49 PM UTC-5, Robert Hafner wrote:

Why are we going back to draft? I thought we were making the one change and then returning to review.

That would be going back to draft. :) Meaning, for that window of time we could consider making other changes that we would otherwise not have made if we didn't go back to review. Larry originally asked if we could change the interface in review; I think we might have been able to but now it does not matter since we can make this change at the same time as the other.

 

I think the exceptions are fine for now, and we as a group will most likely have to revisit exceptions for everything once "throwable" is the standard.

Yeah, I threw that out there hastily as a "wish we had this several years ago so we could rely on this now instead of waiting for PHP 7 as it would make this discussion pointless." 

[Beau Simensen]

Actually, let me be more direct.

Larry, if you can create a PR making this change, we can get it reviewed and ready to go so that once I take PSR-6 out of review we can merge this and the removal of get expiration at the same time and put it back up for review again.

If we get this done in the next few days we can still have the PSR-6 vote finalized well by the end of September.

[Paul Dragoonis]

I see this as a regression in usability. 

1) Naming our class the same as a core PHP class is only going to increase the need for people to make mistakes. i.e: see "throw new InvalidArgumentExcpetion".

2) In phpStorm, your 'use' area is collapsed and if you typed "InvalidArgu" in storm it would autocomplete and give you two options and users can quite easily pick the wrong one.

3) My alternative suggestion to remove this problem is to name it "InvalidCacheArgumentException" meaning the name itself is contextualised and unique.

Thanks.

 

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/2b746f15-1e12-4336-843d-cdbd1c36482b%40googlegroups.com.

[Beau Simensen]

On Friday, August 28, 2015 at 12:27:51 AM UTC-5, Paul Dragoonis wrote:

On Wed, Aug 19, 2015 at 2:27 AM, Beau Simensen <sime...@gmail.com> wrote:

On Tuesday, August 18, 2015 at 8:19:49 PM UTC-5, Beau Simensen wrote:

On Tuesday, August 18, 2015 at 8:09:49 PM UTC-5, Robert Hafner wrote:

Why are we going back to draft? I thought we were making the one change and then returning to review.

That would be going back to draft. :) Meaning, for that window of time we could consider making other changes that we would otherwise not have made if we didn't go back to review. Larry originally asked if we could change the interface in review; I think we might have been able to but now it does not matter since we can make this change at the same time as the other.

 

I think the exceptions are fine for now, and we as a group will most likely have to revisit exceptions for everything once "throwable" is the standard.

Yeah, I threw that out there hastily as a "wish we had this several years ago so we could rely on this now instead of waiting for PHP 7 as it would make this discussion pointless." 

Actually, let me be more direct.

Larry, if you can create a PR making this change, we can get it reviewed and ready to go so that once I take PSR-6 out of review we can merge this and the removal of get expiration at the same time and put it back up for review again.

If we get this done in the next few days we can still have the PSR-6 vote finalized well by the end of September.

I see this as a regression in usability. 

You're objections here are not relevant to the patch in question. The exception was always named `InvalidArgumentException`, the only thing this patch did was make `InvalidArgumentException` extend `CacheException`.

Please "yay" or "nay" having the sub-exceptions extend the common `CacheException`. If you'd like to bring this issue of changing the name up separately on the list, by all means, but for now we're only discussing having the sub-exceptions extend the common `CacheException` interface.

From my perspective, `InvalidArgumentException` is a fine name and we haven't changed this for awhile (that I know of) and the name hasn't been an issue until now (that I know of) so I'd be of the mindset to leave it. Plenty of packages already define an `InvalidArgumentException` class so I don't think that us providing a subtly different name is all that important. If you'd like to bring this issue up separately, by all means, but for now we're only discussing having the sub-exceptions extend the common `CacheException` interface. http://d.pr/i/Cim7

If this is super important to you, you should definitely bring it up on the list as a separate issue on the mailing list. However, I'd suggest making doubly sure you think this is actually all that important enough of a change to you that it be allowed to draw this process out longer.

[Yura Rodchyn]

Hi all,

why not Throwable interface?

[Jonathan Eskew]

The authors were unwilling to use PHP 5.5 features due to the versioning obligations that would impose on consuming packages. I expect that using PHP 7 features would meet with the same resistance.

[Nicolas Ezequiel Cevallos]

Hi there,

Also, Throwable cannot be implemented directly in the classes.

Check the note into this link: http://php.net/manual/en/class.throwable.php



Regards