Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow psr/log 2.0 #1564

Closed
wants to merge 2 commits into from
Closed

Allow psr/log 2.0 #1564

wants to merge 2 commits into from

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Jul 14, 2021

psr/log 2.0 was just tagged: https://twitter.com/phpfig/status/1415413578191024130

3.0 has been tagged too, but it would require a version bump and dropping support for psr/log 1.0, so we'll need to wait for now.

Details are available in the new meta document: https://www.php-fig.org/psr/psr-3/meta/
As always, changes are done in two steps to make a smoother transition path, as per the PHP-FIG evolution bylaw

Copy link
Contributor

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the provide block need an update as well?

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 15, 2021

Oh! Good question... I think it does, but I'm not totally sure it works with something like ^1 || ^2

@stof
Copy link
Contributor

stof commented Jul 15, 2021

The provide rule should indeed be updated to 1.0.0 || 2.0.0

Copy link
Contributor

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monolog's test suite uses Psr\Log\Test\LoggerInterfaceTest which has been removed in php-fig/log#76.

@derrabus
Copy link
Contributor

Should be fixed by #1565.

@derrabus
Copy link
Contributor

3.0 has been tagged too, but it would require a version bump and dropping support for psr/log 1.0

No, it wouldn't. All implementations of the psr/log interfaces that this repository provides already have the necessary return types that are required for 3.0 compatibility. I've run the test suite locally with psr/log 3.0 and it passes (with one error related to an implementation from a different package).

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 20, 2021

@derrabus oh you're right, it's because Monolog already did the major bump to add return types! Good!

@Seldaek Seldaek added this to the 2.x milestone Jul 23, 2021
@Jean85
Copy link
Contributor Author

Jean85 commented Jul 23, 2021

PHPStan failure seems unrelated... Rebased onto main hoping it would fix the CI.

@Seldaek
Copy link
Owner

Seldaek commented Jul 23, 2021

The CI works because psr/log 2.0 isn't used due to transient dependencies:

$ c why-not psr/log 2.0.0
elasticsearch/elasticsearch  v7.13.1  requires  psr/log (~1.0)
graylog2/gelf-php            1.7.0    requires  psr/log (~1.0)
rollbar/rollbar              v1.8.0   requires  psr/log (^1.0.1)
ruflin/elastica              5.1.0    requires  psr/log (~1.0)

But technically yeah this will break the tests once 2.0 is used, so will wait until #1565 is mergeable.

Anyway if you want to help move the ecosystem forward, see the list of packages above :D

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 2, 2021

Done (where missing)!

Let's hope for a quick turnaround...

@ezimuel
Copy link

ezimuel commented Aug 3, 2021

@Seldaek I just released elasticsearch-php 7.14.0 including the psr/log v2 support. Thanks to @derrabus for elastic/elasticsearch-php#1154.

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 27, 2021

Last blockers: ruflin/Elastica#1971 (merged but not tagged yet) and rollbar/rollbar-php#536 (still pending)

@trickeyone
Copy link

Would this be able to also allow for psr/log 3.0? It was tagged on July 14.

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 27, 2021

Would this be able to also allow for psr/log 3.0? It was tagged on July 14.

Not without dropping support for 1.0, which is a huge deal in term of ecosystem support. It will be possible only in the future, once other packages would have catch up with 2.0 at least.

@trickeyone
Copy link

Not without dropping support for 1.0, which is a huge deal in term of ecosystem support. It will be possible only in the future, once other packages would have catch up with 2.0 at least.

Ah, ok. Didn't realize there was that much BC-breaking changes.

@derrabus
Copy link
Contributor

Would this be able to also allow for psr/log 3.0? It was tagged on July 14.

Not without dropping support for 1.0

Again, I disagree. We've had that discussion already. 🙂

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 31, 2021

Not without dropping support for 1.0

Again, I disagree. We've had that discussion already. slightly_smiling_face

Sorry but I forgot about the discussion... And while searching for that I found the reason! 😄 all return types in psr/log are void, so that's a special case that doesn't trigger Liskov incompatibility as in the general case! 🎉

See php-fig/log#77

[EDIT] Still, we can't force it downstream, and we're blocked by them to have a CI that runs all the checks.

@derrabus
Copy link
Contributor

all return types in psr/log are void

… and already implemented by Monolog.

[EDIT] Still, we can't force it downstream, and we're blocked by them to have a CI that runs all the checks.

At some point, we run into a vicious circle of blockers. Monolog is a key implementation for psr/log. If we don't allow the new interfaces here, a large share of the ecosystem is blocked.

If the CI is really a problem, we could add a reduced CI job that only runs core tests and ignores dependencies that are not ready yet.

@Seldaek
Copy link
Owner

Seldaek commented Sep 14, 2021

Closing as this commit was included in #1587 - thanks everyone!

@Seldaek Seldaek closed this Sep 14, 2021
@Jean85 Jean85 deleted the patch-1 branch September 16, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants