Add support for Symfony 6

symfony / panther

A browser testing and web crawling library for PHP and Symfony

MIT License

2.93k stars 220 forks source link

Add support for Symfony 6 #509

Closed jordisala1991 closed 2 years ago

jordisala1991 commented 2 years ago

It is required to run with dev dependencies, otherwise it wont be installed. I didn't found a dev dependencies build so I added one (if it is not correct this way, I can revert it or change to another one)

jordisala1991 commented 2 years ago

This one is strange:

PHP Fatal error:  Declaration of Symfony\Component\Panther\DomCrawler\Crawler::eq($position): Symfony\Component\Panther\DomCrawler\Crawler must be compatible with Symfony\Component\DomCrawler\Crawler::eq(int $position): Symfony\Component\ErrorHandler\DebugClassLoader in /home/runner/work/panther/panther/src/DomCrawler/Crawler.php on line 89

jordisala1991 commented 2 years ago

IMO the contents of this commit: https://github.com/symfony/panther/pull/509/commits/31d379ed84cc7cb041ce2e4ebedeeda448d0a2b3 should be reverted, because it makes this bundle not compatible with php < 8.0 (that commits belongs to this PR). Currently without that commit this PR do not pass the new build I added for Symfony 6

@wouterj do you agree? Should I open one PR to revert this return typehint on the main Symfony repository? Are multiple PRs needed or just one (IMO it should be better in one, because all of them have to be accepted for this PR to continue, otherwise php < 8 have to be dropped to accept Symfony 6)?

jordisala1991 commented 2 years ago

I don't think the failure is related to my changes.

Edit: A restart fixed the build

nicolas-grekas commented 2 years ago

What about bumping panther to v2?

jordisala1991 commented 2 years ago

What about bumping panther to v2?

The work to be done is mostly the same, removing only the last commit cleans up the return types to be compatible only with php 8 and sf 6.

There are some things to consider

What other changes are needed for v2? Maybe deprecate more things or cleaning up some stuff.
The way forward for bundle maintainers will be requiring ^1 and ^2 if they want to be compatible with php 7 and sf 5

Maybe there are more things that I cant see because I dont know a lot of this bundle.

nicolas-grekas commented 2 years ago

For this bundle I agree. For for Symfony that'd allow withdrawing symfony/symfony#44005, which could be nice. /cc @dunglas WDYT?

chalasr commented 2 years ago

Works for me. Should we maintain a 1.x branch?

dunglas commented 2 years ago

I've no strong opinion about this. I'm ok with both options.

jordisala1991 commented 2 years ago

So, which are the next steps?

weaverryan commented 2 years ago

I’m hearing a slight preference to make a v2 of this bundle. That would be my vote as well so we can move things forward.

As far as a 1.x branch? We could decide on that later, but there is little harm to create one.

chalasr commented 2 years ago

@jordisala1991 Could you revert the DomCrawler patch to make the code PHP8+ only, then mark this PR as ready?

jordisala1991 commented 2 years ago

Done @chalasr

weaverryan commented 2 years ago

@dunglas I believe you're needed to merge and tag? I would love that - it would unblock a Symfony6 release for symfony/ux... which would unblock some other symfony/ux stuff :).

dunglas commented 2 years ago

Merged! Thanks @jordisala1991!

@weaverryan I'll try to craft a release on Monday.