Skip to content

Implement CacheableSupportsMethodInterface introduced in Symfony 4.1#1988

Merged
dunglas merged 3 commits into
api-platform:masterfrom
dunglas:serializer-41
Jun 1, 2018
Merged

Implement CacheableSupportsMethodInterface introduced in Symfony 4.1#1988
dunglas merged 3 commits into
api-platform:masterfrom
dunglas:serializer-41

Conversation

@dunglas

@dunglas dunglas commented May 30, 2018

Copy link
Copy Markdown
Member
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

It should improve dramatically the performance (benchmarking in progress).
It also means that we drop support for the Serializer Component <4.1, and as a direct consequence of PHP 7.0.

@dunglas dunglas force-pushed the serializer-41 branch 3 times, most recently from 5d8c37e to b4e78c1 Compare May 31, 2018 08:44
@dunglas

dunglas commented May 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Upgrading from Symfony 4.0 to 4.1 (with the patch in this PR included) makes API Platform apps 41% faster, and reduce the used memory of 41% too!
Here is a Blackfire comparison: https://blackfire.io/profiles/compare/b4ff2e86-8d10-4419-b5f1-2609dc247ff6/graph

To create this profile, I updated the demo application to add support for most new features: api-platform/demo#24
Then I loaded the app's fixtures and executed this CURL request:

blackfire curl -G http://localhost:8080/books \
    -d "properties[]=isbn" \
    -d "properties[]=title" \
    -d "properties[]=description" \
    -d "properties[]=author" \
    -d "properties[]=publicationDate" \
    -d "properties[reviews][]=rating" \
    -d "properties[reviews][]=body" \
    -d "properties[reviews][]=author" \
    -d "properties[reviews][]=publicationDate"

dunglas
dunglas commented May 31, 2018
View reviewed changes
Comment thread src/Identifier/Normalizer/DateTimeIdentifierDenormalizer.php
try {
return parent::denormalize($data, $class, $format, $context);
} catch (InvalidArgumentException $e) {
} catch (NotNormalizableValueException $e) {

@dunglas dunglas May 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@soyuka, I've found this bug while adding an unit test. Can you check that it's correct please?

@soyuka soyuka Jun 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh yes I used the normalize signature to find the exception. This is indeed correct.

soyuka
soyuka reviewed Jun 1, 2018
View reviewed changes
Comment thread tests/Identifier/Normalizer/DateTimeIdentifierNormalizerTest.php
/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class DateTimeIdentifierNormalizerTest extends TestCase

@soyuka soyuka Jun 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we had absolutely no tests on these? I'm sorry I didn't thought about it haha.

@dunglas dunglas merged commit 0d31d5c into api-platform:master Jun 1, 2018
@dunglas dunglas deleted the serializer-41 branch June 1, 2018 09:29
bendavies
bendavies reviewed Jun 1, 2018
View reviewed changes
Comment thread tests/Identifier/Normalizer/DateTimeIdentifierNormalizerTest.php
class DateTimeIdentifierNormalizerTest extends TestCase
{
/**
* @expectedException \ApiPlatform\Core\Exception\InvalidIdentifierException

@bendavies bendavies Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

@soyuka soyuka Jun 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interesting, we're using the annotation everywhere though so I guess it's fine :)

@teohhanhui teohhanhui Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do that in another PR to change it all at one go. 😄

@dunglas dunglas Jun 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know about this article, it's interesting, but if we change, we must change everywhere in the lowest maintained branch.

@teohhanhui teohhanhui Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not a problem to change it, right? Not a BC break.

@dunglas dunglas Jun 1, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed. Just some work that is not top priority for me :D

@bendavies bendavies Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#1993

bendavies
bendavies reviewed Jun 1, 2018
View reviewed changes
Comment thread tests/Identifier/Normalizer/DateTimeIdentifierNormalizerTest.php
/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class DateTimeIdentifierNormalizerTest extends TestCase

@bendavies bendavies Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add a supportsDenormalization test?

@dunglas dunglas Jun 1, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not necessary, the supportsDenormalization method is not overrode and the base one is already tested in Symfony.

@bendavies bendavies Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah yes

bendavies
bendavies reviewed Jun 1, 2018
View reviewed changes
Comment thread tests/Identifier/Normalizer/IntegerDenormalizerTest.php
/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class IntegerDenormalizerTest extends TestCase

@bendavies bendavies Jun 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test for hasCacheableSupportsMethod

@dunglas dunglas Jun 1, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's done line 35

@bendavies bendavies mentioned this pull request Jun 1, 2018
Merged
@bendavies

bendavies commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

@dunglas can't this go on 2.2?

@soyuka

soyuka commented Jun 8, 2018

Copy link
Copy Markdown
Member

looks like it's tight to symfony 4.1 and php 7.1 so not really

@bendavies

bendavies commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

yeah, i meant the CacheableSupportsMethodInterface changes really, as 2.2 allows symfony 4.1

@teohhanhui

teohhanhui commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

If it's possible to implement CacheableSupportsMethodInterface without any BC breaks, sure.

@soyuka

soyuka commented Jun 8, 2018

Copy link
Copy Markdown
Member

I'm not sure that it can be considered as a bug fix though :p

@teohhanhui

teohhanhui commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

Fixing the bad performance. How about that? 😜

@dunglas

dunglas commented Jun 8, 2018

Copy link
Copy Markdown
Member Author

I'm not confident to merge this in 2.2. Anyway I'll release 2.3 beta the next week.

@teohhanhui teohhanhui mentioned this pull request Jun 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soyuka soyuka soyuka approved these changes
+2 more reviewers
@teohhanhui teohhanhui teohhanhui left review comments
@bendavies bendavies bendavies left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dunglas @bendavies @soyuka @teohhanhui