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

feat: Add symfony 6 compatibility #4582

Merged
merged 9 commits into from
Dec 23, 2021
Merged

feat: Add symfony 6 compatibility #4582

merged 9 commits into from
Dec 23, 2021

Conversation

PierreRebeilleau
Copy link
Contributor

@PierreRebeilleau PierreRebeilleau commented Dec 1, 2021

Q A
Branch? 2.6
Tickets #4543, #4581
License MIT
Doc PR no

Test are failing because of some type hinting, waiting for a new release of symfony 6 , solved by symfony/symfony#44331

@PierreRebeilleau PierreRebeilleau changed the title Add symfony 6 compatibility feat: Add symfony 6 compatibility Dec 1, 2021
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Can you also allow the latest versions of the PSR packages please?

composer.json Outdated Show resolved Hide resolved
@pietereggink
Copy link

symfony/symfony#44331 is merged, is it possible to execute the quality checks again?

@rkeet
Copy link

rkeet commented Dec 9, 2021

Hi, was looking if I could help out a bit so did some digging through requirements in composer.json as I noticed it still has SF 3.4 in there.

SF 3.4 LTS is now past EOL (since this month).

Also, SF 3.4 has "php": "^5.5.9|>=7.0.8" (link) for requirements, while ApiPlatform has ">=7.1" (link) (not changed in this PR). Not sure if that would cause issues with compatibility, but maybe a removal of SF 3.4 of supported versions is also an idea for this PR, since you're updating with SF6?

Extra question: is there somewhere I can see/look into/help efforts for a possible v3 and/or minimum PHP 8 / 8.1 requirement?

@dunglas
Copy link
Member

dunglas commented Dec 13, 2021

I would prefer to drop Symfony 3.4/PHP 7.1 in the main branch. But if it's too annoying to keep supporting them, then let's bump these minimal versions.

The main branch contains the 2.7 version, which will be the same code as 3.0 but with a backward compatibility layer for API Platform 2 (that we'll drop before tagging 3.0).

src/Bridge/Symfony/Bundle/Test/ApiTestCase.php Outdated Show resolved Hide resolved
@@ -125,6 +129,10 @@ protected function setAttributeValue($object, $attribute, $value, $format = null
$attribute = 'id';
}

if ('{}' === $value) {
Copy link
Member

Choose a reason for hiding this comment

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

That's weird. Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we dump dump($context['args']['input']); in the DeserializeStage we got this :

array:4 [
  "id" => "/related_dummies/2"
  "symfony" => "laravel"
  "thirdLevel" => "/third_levels/1"
  "embeddedDummy" => "{}"
]

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bug in pho-GraphQL. Could you report it upstream and explain why we need this temporary fix in a comment please?

Copy link
Member

Choose a reason for hiding this comment

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

Input is user-defined, there shouldn't be a fix in the serializer. Maybe the test is bad. Which test is failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is strange, we need to dig it, it's probably not the right fix.

@@ -808,49 +808,18 @@ public function testDisabledPaginationViaDefaults()

public function testItLoadsMetadataConfigFileInAlphabeticalOrder()
{
$yamlExtractorDefinition = $this->prophesize(Definition::class);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test removed?

@alanpoulain alanpoulain merged commit 5b91f40 into api-platform:2.6 Dec 23, 2021
@alanpoulain
Copy link
Member

Thank you everyone.

@dunglas
Copy link
Member

dunglas commented Dec 23, 2021

You rock!

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.

7 participants