CacheException / Throwable

[ElGigi]

Extends CacheException with \Throwable interface.
With PHP 5 backward compatibility.

A do an error on pull request : #12 ; sorry ;)

[fnoir]

Can you merge the pull request?

[danopz]

Did you test it? It is not working: https://3v4l.org/kgWmT
Nevermind

[prisis]

Its works, your example is wrong https://3v4l.org/7NPYS

A class cant extend a Throwable interface you always need a Exception class

[ElGigi]

Any news ?

[fnoir]

@michaelcullum @dragoonis Is this PR mergeable ? Thx

[francislavoie]

@Seldaek @michaelcullum any chance this could be looked at? PHPStan gives type errors, this change would fix it.

PHPDoc tag @throws with type Psr\SimpleCache\InvalidArgumentException is not subtype of Throwable  

[c33s]

/ping @weaverryan

[glensc]

The problem here is that \Psr\SimpleCache\InvalidArgumentException is an interface, not an \Exception (or \Throwable) subclass.

so if you intend to throw, you need to declare your exception class:

namespace MyCache;

class InvalidArgumentException extends \RuntimeException implements \Psr\SimpleCache\InvalidArgumentException {}

Don't know why it is so, probably to allow use any \Exception subclass?

but it allows actual exception to extend any of the \Exception subclass:

namespace MyCache;

class InvalidArgumentException extends \Exception implements \Psr\SimpleCache\InvalidArgumentException {}
class InvalidArgumentException extends \RuntimeException implements \Psr\SimpleCache\InvalidArgumentException {}
class InvalidArgumentException extends \Throwable implements \Psr\SimpleCache\InvalidArgumentException {}

I think it would still make sense to make the class in this project extend \Exception then actual implementations can throw the specific exception from this project, without needing to create new exception class.

[glensc]

you are not solving the problem here, ThrowablePhp5 is still just an interface, not \Exception subclass.

this should probably be class ThrowablePhp5 extends \Exception

[ElGigi]

CacheException is an interface, so can't extends a class.
That's solve the problem for PHP 7.

[PetrHeinz]

@glensc For PHP 5 there is no way to solve the problem as there is no interface for Exceptions. There shouldn't be any implementations as the repository only contains interfaces. The ThrowablePhp5 is there only for BC.

[xelan]

As the CacheException is just a marker interface for cache exception implementations, the latter must extend the PHP base Exception anyway, which is implementing Throwable on PHP 7.0+. Or am I missing something here?

[Jean85]

We've been discussing this kind of issues a lot lately, and we've condensed our proposed solution to updating PSR interfaces in a blog post. Please read it and provide your feedback!

https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

[PetrHeinz]

Is there anything preventing this PR to be merged? It is backward-compatible as any class that can be thrown has to be Throwable as of PHP 7 and there is ensured compatibility with lower versions via a marker interface ThrowablePhp5.

@Jean85 I've read the chapter about the problem and it doesn't apply here. There are no types added.

Ping @Seldaek

[Jean85]

Now we have a proper bylaw and procedure to handle this kind of cases: https://www.php-fig.org/bylaws/psr-evolution/

We could handle this with a bump to the PHP requirement to (at least) ^7.0 and the direct usage of extends \Throwable, and release it as 1.1. That seems a good compromise for me, but I would take the chance to see if there are other upgrades that we could add at the same time.

[PetrHeinz]

@Jean85 Thanks, that makes sense 👍

[ElGigi]

So i removed PHP5 compatibility 😄

[c33s]

@Jean85 as far as i read the change in this pull request is BC compatible with php5. why not keep it compatible with php5 and have the fixed behavior for php7?

[Jean85]

@Jean85 as far as i read the change in this pull request is BC compatible with php5. why not keep it compatible with php5 and have the fixed behavior for php7?

This change would be released as a new minor version, so whoever uses it on PHP 5 would still rely on the 1.0 release of this package. That would leave PHP 7 users with the fix, which would impact mainly static analysis and correct implementation of the spec, not runtime.

[dragoonis]

Hey everyone! Thanks for commenting here.

FYI: The authors of this standard are me, and @Seldaek, so there's no use in pinging other people.

Please continue to ping @Jean85 as it's his responsibility to review community stuff.

I'm sorry that you're seeing issues with Stan. I think it's a legitimate time to review this implementation.

When I created this standard, PHPStan wasn't a mature library in PHP ecosystem and I didn't run it against it.

Back in 2019 when people were looking into changes, the FIG's upgrade policy wasn't matured or trialed out either, so this is why I didn't consider making any changes to this at that time.

[dragoonis]

Technical Review:
When I created this standard, PHP5 / PHP7 compatibility was important for me, and I didn't want to include PHP7 features when, at the time, PHP7 was still being adopted into the community. Therefore, I left out any kind of PHP7-like implementation details in these interfaces.

I can see that PHP7 has added Throwable, which PHPStan has decided to complain about. The issue here is I can't change CacheInterface to extend Throwable, because it indeed doesn't exist in PHP5.

What the community is asking for, is a dual-compatible implementation between PHP5 and PHP7, and using composer to handle the "figuring out" of which version of the library to use.

This means the FIG will need to consider maintaining 2 versions of this, and if you don't configure composer correctly, then you'll get the wrong version.

I'd like to hear from @Jean85 on what certainties he can provide me with, that the decided approach going forward, is safe, for people who don't specify the PHP version in their composer, and for developers who aren't running composer update/install on the production target environment, but instead their local machine (i.e: --ignore-platform-reqs).

I hope this made sense, and I'm looking forward to hearing what ideas and safety measures can be put in place, to make sure PHP5 consumers already using this library are not going to get the wrong version, but people long PHP7 can "explicitly" say they want the new version.

Sorry if it's taken a long time for me to see this thread, you're all welcome to ping me on twitter to get my attention: https://twitter.com/dr4goonis

Cheers, viva la PHP.

[Jean85]

I'd like to hear from @Jean85 on what certainties he can provide me with, that the decided approach going forward, is safe, for people who don't specify the PHP version in their composer, and for developers who aren't running composer update/install on the production target environment, but instead their local machine (i.e: --ignore-platform-reqs).

In my opinion, whoever is doing composer update --ignore-platform-reqs without even using the platform directive is Doing It Wrong(TM). Sorry, but misusing a tool can't be our concern while handling a standard IMO. And that's the spirit in which I intended that evolution bylaw when I proposed it.

So yes, that's the only pitfall in declaring the new version as 1.1 with PHP ^7.0; but doing otherwise (like releasing as 2.0) would create a huge rift in the community, requiring a lot of distributed effort in implementing libraries to make all cross compatible again, which would put us back to breaking projects again when you do --ignore-platform-reqs, since end users do not rely on PSR packages directly.

[Seldaek]

👍 from me on this and other 7/7.1 changes to bring the PSR forward. But I won't have time to do any merging or further work here, if someone else does please do.

[dragoonis]

@Seldaek I'll hold the fort :)

[dragoonis]

@Jean85 I'm happy with this approach as long as it's a safe upgrade, and users don't need special flags in their composer.json files or on the composer CLI.

1. If their project doesn't specify which PHP version they're looking for, this will default to PHP 5.x version, am I right? Please confirm.

2. If they specify php 7.0+, they'll the latest version, am I right? Please confirm.

[Jean85]

It's not the specified project that will lock onto the right version, but the version of PHP in use during composer update. That can be overridden with the config.platform.php directive in composer.json if you do not have a proper dev environment.

So, if you have PHP 7+, you'll get the new version. If you do not, you'll get the old one.

As I said in a previous comment (#13 (comment)), if something breaks, at that point it's the user's fault.

PS: just a small reminder, we need to pass this through the bylaw procedure (#13 (comment)), so it would make sense to group all the meaningful PRs here and do a single vote and release.

[dragoonis]

@Jean85 reach out to me on the PHPFIG slack for a conversation on the approach forward? Thanks

[neclimdul]

so... its 2021. Can this be merged yet?

[Jean85]

PSR-6 received a similar upgrade just last month, see:

• https://www.php-fig.org/psr/psr-6/meta/#82-type-additions
• https://twitter.com/phpfig/status/1357237647031304194
• php-fig/cache@1.0.1...2.0.0
• php-fig/cache@2.0.0...3.0.0

@dragoonis you can look at how that was done (and how we're doing it for PSR-11) as an example; we should talk about this on the new PHP-FIG Discord: https://discord.gg/php-fig

[Jean85]

Done in dd145de inside #25 (pending).