Add support for Symfony 6 #509

jordisala1991 · 2021-10-30T08:37:41Z

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 · 2021-10-30T09:02:13Z

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 · 2021-10-30T14:16:24Z

IMO the contents of this commit: 31d379e 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)?

src/WebTestAssertionsTrait.php

weaverryan · 2021-11-08T01:01:17Z

src/DomCrawler/Crawler.php

@@ -86,7 +86,7 @@ public function addNode(\DOMNode $node): void
        throw $this->createNotSupportedException(__METHOD__);
    }

-    public function eq($position): self
+    public function eq($position): static


This is another spot where we can't do this... unless we're planning to drop support for PHP 7. But... maybe we are??? In order to support Symfony 6?

I guess the core team of Symfony should take a decision on this. I can work on both implementations. Keeping support for older php version needs main Symfony repository to be adapted before 6.0 comes out. Removing support for older php versions might need a major version of this package.

But indeed this one I missed when reporting here: symfony/symfony#43021

This will be a ton of changes, because all of. this class changed its return types. 😱

Either we remove the return types from core, or we add some conditional classes/traits declarations in this bundle.
I'm personally ok to revert the addition of these type declarations, we can re-add them later.

So, should we revert these in core? If we're going to do that, I guess we should do it as quickly as we can.

(I'm still very unfamiliar with the typing situation - so apologies for not having anything intelligent to add)

Yes, I am trying to fix everything here to know what types exactly should be reverted, because I wasn't taking care of static.

My idea is:

Prepare this PR to be able to ensure that changes on Symfony will make this PR pass

Make a PR on Symfony core to change all types needed here

Adapt this PR when the PR on the main repo is merged

For the moment I am having some trouble with PHPStan, since it is not a dependency of this project it got upgraded to 1.0 without notice and now it reports more errors.

This makes sense to me - thank you and keep us posted :)

Im trying a little trick to patch symfony to remove its return types so we can ensure that those changes applied to Symfony 6.0 will make build pass with all versions here.

So far I think it will work, ~~but I am having some last troubles with PHPStan.~~ No more problems with PHPStan, it is working.

The idea is to remove the last 2 commits from this PR once I do the Symfony PR to remove the return typehints

wdyt?

jordisala1991 · 2021-11-10T20:11:59Z

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

Edit: A restart fixed the build

weaverryan

I'll need someone more familiar with the "types" topic on the Symfony core to take a look, but this makes sense to me - very nice work!

nicolas-grekas · 2021-11-11T15:35:32Z

What about bumping panther to v2?

jordisala1991 · 2021-11-11T17:43:01Z

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 · 2021-11-11T17:50:04Z

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

chalasr · 2021-11-11T17:54:31Z

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

dunglas · 2021-11-11T21:12:02Z

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

jordisala1991 · 2021-11-11T21:35:45Z

So, which are the next steps?

weaverryan · 2021-11-14T17:55:40Z

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 · 2021-11-16T11:09:54Z

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

jordisala1991 · 2021-11-17T07:56:18Z

Done @chalasr

weaverryan · 2021-11-17T16:41:02Z

.github/workflows/ci.yml

+                  restore-keys: ${{ runner.os }}-composer-
+
+            - name: Allow dev dependencies
+              run: composer config minimum-stability dev


Normally we have minimum-stability: "dev" in composer.json by default... and that's what we always use. If we did that, then we wouldn't need this second job, I think. For example: /~https://github.com/symfony/string/blob/d70c35bb20bbca71fc4ab7921e3c6bda1a82a60c/composer.json#L39

A little more info. What we normally do is:

Put "minimum-stability": "dev" directly in composer.json

For "normal" builds (e.g. released versions like Symfony 5.3), we use --prefer-stable with composer update.

For non-released builds (e.g. Symfony 6), we would omit the --prefer-stable.

BUT, I think these are not-important details. The current builds DO test the correct stuff. I vote we merge this, and then we could potentially optimize this build file later to reduce duplication.

I think it would be nice to have a better testing of different symfony versions, as you said it can be done on another Pr. I can bring some of the workflows we use at Sonata if you dont have a standard way for all sf repositories.

weaverryan

I'm good with this - for a 2.0 release.

A CHANGELOG entry for 2.0 would be great.

.github/workflows/ci.yml

weaverryan · 2021-11-19T18:02:53Z

@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 · 2021-11-19T18:50:53Z

Merged! Thanks @jordisala1991!

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

jordisala1991 added 2 commits October 30, 2021 10:35

Allow Symfony 6

e4d2771

Test with dev dependencies

5a1779f

jordisala1991 force-pushed the feature/symfony-6 branch 4 times, most recently from 08406f7 to 8d968f8 Compare October 30, 2021 08:59

jordisala1991 force-pushed the feature/symfony-6 branch 15 times, most recently from dfc9c94 to 31d379e Compare October 30, 2021 12:47

jordisala1991 commented Oct 30, 2021

View reviewed changes

src/WebTestAssertionsTrait.php Show resolved Hide resolved

This was referenced Oct 31, 2021

[META] Symfony 6 return type declarations symfony/symfony#43021

Closed

Symfony 6 Support sonata-project/dev-kit#1675

Closed

weaverryan mentioned this pull request Nov 8, 2021

Allowing Symfony 6 symfony/ux#160

Merged

weaverryan reviewed Nov 8, 2021

View reviewed changes

jordisala1991 force-pushed the feature/symfony-6 branch 2 times, most recently from 9e65672 to 698ced4 Compare November 10, 2021 08:08

jordisala1991 requested a review from weaverryan November 10, 2021 20:03

jordisala1991 closed this Nov 10, 2021

jordisala1991 reopened this Nov 10, 2021

jordisala1991 mentioned this pull request Nov 11, 2021

[BrowserKit][DomCrawler] Relax return types needed for Symfony Panther symfony/symfony#44005

Closed

weaverryan approved these changes Nov 11, 2021

View reviewed changes

kbond mentioned this pull request Nov 13, 2021

Symfony 6 support zenstruck/browser#64

Merged

1 task

jordisala1991 force-pushed the feature/symfony-6 branch from 2e57792 to e5e7ed6 Compare November 17, 2021 07:45

jordisala1991 added 2 commits November 17, 2021 08:46

only test with PHP 8.0

73278bf

Remove this - Problematic changes

2854fc9

jordisala1991 marked this pull request as ready for review November 17, 2021 07:49

jordisala1991 requested a review from chalasr November 17, 2021 07:56

weaverryan reviewed Nov 17, 2021

View reviewed changes

weaverryan approved these changes Nov 17, 2021

View reviewed changes

chalasr approved these changes Nov 17, 2021

View reviewed changes

dunglas approved these changes Nov 18, 2021

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

Minimum version php 8.0

a0889fa

dunglas merged commit 9917f08 into symfony:main Nov 19, 2021

jordisala1991 deleted the feature/symfony-6 branch November 19, 2021 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Symfony 6 #509

Add support for Symfony 6 #509

jordisala1991 commented Oct 30, 2021

jordisala1991 commented Oct 30, 2021

jordisala1991 commented Oct 30, 2021 •

edited

Loading

weaverryan Nov 8, 2021

jordisala1991 Nov 8, 2021

jordisala1991 Nov 8, 2021

chalasr Nov 8, 2021

weaverryan Nov 9, 2021

jordisala1991 Nov 10, 2021

weaverryan Nov 10, 2021

jordisala1991 Nov 10, 2021 •

edited

Loading

jordisala1991 commented Nov 10, 2021 •

edited

Loading

weaverryan left a comment

nicolas-grekas commented Nov 11, 2021

jordisala1991 commented Nov 11, 2021

nicolas-grekas commented Nov 11, 2021

chalasr commented Nov 11, 2021

dunglas commented Nov 11, 2021

jordisala1991 commented Nov 11, 2021

weaverryan commented Nov 14, 2021

chalasr commented Nov 16, 2021

jordisala1991 commented Nov 17, 2021

weaverryan Nov 17, 2021

weaverryan Nov 17, 2021

jordisala1991 Nov 17, 2021 •

edited

Loading

weaverryan left a comment

weaverryan commented Nov 19, 2021

dunglas commented Nov 19, 2021

Add support for Symfony 6 #509

Add support for Symfony 6 #509

Conversation

jordisala1991 commented Oct 30, 2021

jordisala1991 commented Oct 30, 2021

jordisala1991 commented Oct 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordisala1991 Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

jordisala1991 commented Nov 10, 2021 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 11, 2021

jordisala1991 commented Nov 11, 2021

nicolas-grekas commented Nov 11, 2021

chalasr commented Nov 11, 2021

dunglas commented Nov 11, 2021

jordisala1991 commented Nov 11, 2021

weaverryan commented Nov 14, 2021

chalasr commented Nov 16, 2021

jordisala1991 commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordisala1991 Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan commented Nov 19, 2021

dunglas commented Nov 19, 2021

jordisala1991 commented Oct 30, 2021 •

edited

Loading

jordisala1991 Nov 10, 2021 •

edited

Loading

jordisala1991 commented Nov 10, 2021 •

edited

Loading

jordisala1991 Nov 17, 2021 •

edited

Loading