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

Prepare for deprecations #66

Merged
merged 7 commits into from
Jul 5, 2023
Merged

Prepare for deprecations #66

merged 7 commits into from
Jul 5, 2023

Conversation

ramondelafuente
Copy link
Contributor

Hey @mnapoli,

fixes #65

I've looked at dealing with deprecations and I'd like to hear what you think. I've added symfony/phpunit-bridge to bring depreaction notices to the surface. As per the documentation for libraries I configured it to trip only on deprecations within the project.

I also added the <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/> to improve the output for the deprecations (see screenshots).

without the testlistener

with the testlistener


Two of the deprecations are simple enough, and I have added the fix in this PR. It should not break BC.

  • add void return type to Kernel::boot()
  • explicitly set the framework.http_method_override to the future default value of "false"

The final depreaction is a little trickier - it looks like there will be a "mixed" return type on the PSR-11's container::get() interface link. Since this is not availabel in PHP7.4 I did not fix that. It also occurs in Bref\Runtime\FileHandlerLocator which is not part of this library.

Since I'm not sure if you want to add this phpunit-bridge at all, let me know if you want a PR without it.

@mnapoli
Copy link
Member

mnapoli commented Jun 5, 2023

I have PTSD from the phpunit bridge 😅 but why not if it's not messing up the workflow too much!

We can safely drop support for PHP 7, Bref doesn't support it anymore anyway. Does that help?

@ramondelafuente
Copy link
Contributor Author

ramondelafuente commented Jun 5, 2023

By dropping support for PHP7.4 this change now targets a next major version of this lib.
[Edit: unless of course there are no major versions yet 😅 ]

In which case dropping the unsupported Symfony 5.2 version might also be a good idea.

@mnapoli
Copy link
Member

mnapoli commented Jun 8, 2023

@ramondelafuente let's target a 1.0 release.

Do we gain anything by dropping support for Symfony 5.2? If not, I'd rather keep it just in case.

@ramondelafuente
Copy link
Contributor Author

I missed a reference to PHP7.4 in the static analysis github actions workflow - which I now fixed. But I might need some help with the "lowest deps" failure. It looks like I now also potentially opened the door to a PHPUnit upgrade (it seems coverage does not work on PHP8.0) which is out of scope as far as I'm concerned.

@ramondelafuente
Copy link
Contributor Author

@ramondelafuente let's target a 1.0 release.

Do we gain anything by dropping support for Symfony 5.2? If not, I'd rather keep it just in case.

Nope, nothing to gain - except potential support questions for a Symfony version that is itself no longer supported. It looks pretty calm in the issue list though.

@mnapoli
Copy link
Member

mnapoli commented Jun 10, 2023

For lowest deps what I usually do is increase progressively the required version of dependencies until testing with lowest deps works 😬

@ramondelafuente
Copy link
Contributor Author

I don't understand why the lowest deps fail to be honest.

With a value for PHP_VERSION of 7.4 the commands seem to run on docker just fine. The image name is bref/php-74.
After my change to a version of 8.0 the commands fail with "Unable to find image 'bref/php-8:latest' locally
docker: Error response from daemon: pull access denied for bref/php-8, repository does not exist". The image name is bref/php-80.

I thought I could try to put the PHP_VERSION in quotes because it might be interpreted as "8" somewhere in the process... unless you understand what's happening?

@mnapoli
Copy link
Member

mnapoli commented Jun 12, 2023

That's weird indeed! Just like if 8.0 was rounded to 8 🤔

@mnapoli
Copy link
Member

mnapoli commented Jul 5, 2023

🎉 haha nicely spotted!

Thanks!

@mnapoli mnapoli merged commit f3a15f8 into brefphp:master Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a return type "void" to Kernel::boot() for future Symfony compatibility
2 participants