NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[PropertyInfo] Fix an error in PropertyInfoCacheExtractor#19437

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge2 commits intosymfony:3.1fromGuilhemN:patch-3

Closed

[PropertyInfo] Fix an error in PropertyInfoCacheExtractor#19437

GuilhemN wants to merge2 commits intosymfony:3.1fromGuilhemN:patch-3

Conversation

Copy link

Contributor

GuilhemN commentedJul 26, 2016•
edited
Loading

Q	A
Branch?	3.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

An argument was forgotten in thePropertyInfoCacheExtractor class leading to exceptions such as

PHP Warning:  ucfirst() expects parameter 1 to be string, array given in /home/guilhem/github/ast-test/vendor/symfony/symfony/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php on line 318

and making it unusable.

ping@dunglas

carsonbot added Status: Needs Review PropertyInfo Bug labels

Jul 26, 2016

Copy link

Member

dunglas commentedJul 26, 2016

👍

Can you add a non-regression test please?

GuilhemN force-pushed thepatch-3 branch 4 times, most recently from2e351de to70a8de2Compare

July 26, 2016 17:35

Copy link

ContributorAuthor

GuilhemN commentedJul 26, 2016

@dunglas done :)
BTW this shows how much php7 type hints are useful ! I hope symfony 4 will remove php5 support.

Copy link

Contributor

theofidry commentedJul 27, 2016

BTW this shows how much php7 type hints are useful !

Or any good static code analyzer for that matter :P

Copy link

ContributorAuthor

GuilhemN commentedJul 27, 2016

@theofidry yes of course (but it is slower) :P

Copy link

ContributorAuthor

GuilhemN commentedJul 27, 2016•
edited
Loading

I found another bug but I don't know why it happens...
This condition is nevertrue and the psr cache is always used.
Usingarray_key_exists solves this issue. Any idea ?

Edit: Ok it happens when the extractor returnsnull. I'll submit a commit.

Copy link

Contributor

theofidry commentedJul 27, 2016

did you try to dump the value?

Copy link

Contributor

theofidry commentedJul 27, 2016

@Ener-Getick any idea of what the isset was not working?

Copy link

ContributorAuthor

GuilhemN commentedJul 27, 2016

@theofidry yes sorry i forgot to answer you as i updated my message...
Isset is kind of equals tonull !== @$value so when the value cached isnull,isset returnsfalse whereasarray_key_exists which returns true as long as the value exists.

Copy link

Contributor

theofidry commentedJul 27, 2016

IIRCnull is not cached in most cache systems, shouldn't we avoid to cache it here as well?

Copy link

ContributorAuthor

GuilhemN commentedJul 27, 2016•
edited
Loading

@theofidry it is cached in the cross requests cache but it is not fetched in the array cache which is only here because it is faster than the first one. So it will be cached anyway the only thing changing are the performances.

GuilhemN closed this

Jul 27, 2016

GuilhemN reopened this

Jul 27, 2016

nicolas-grekas reviewed

Jul 28, 2016

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Fixtures/NullExtractor.php Outdated

		privatefunctionassertIsString($string)
		{
		if (!is_string($string)) {
		thrownew \Exception(sprintf('"%s" expects strings, given "%s".',__CLASS__,gettype($string)));

Copy link

Member

nicolas-grekasJul 28, 2016

Choose a reason for hiding this comment

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

InvalidArgumentException?

Copy link

ContributorAuthor

GuilhemNJul 29, 2016

Choose a reason for hiding this comment

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

It doesn't matter that much as it's a fixture but it's indeed more adapted here.
updated :)

GuilhemN force-pushed thepatch-3 branch from3492769 to70f4440Compare

July 29, 2016 18:48

GuilhemN added2 commits

July 29, 2016 20:50

[PropertyInfo] Fix an error in PropertyInfoCacheExtractor

0fe4be9

[PropertyInfo] Fix array caching

7bc58bf

`null` values aren't cached because of the use of `isset`

GuilhemN force-pushed thepatch-3 branch from70f4440 to7bc58bfCompare

July 29, 2016 18:50

Copy link

ContributorAuthor

GuilhemN commentedAug 10, 2016

Should I change something else ?

Copy link

Member

dunglas commentedAug 11, 2016

Ok for me. I'll merge it when I'll be back from vacation.

Copy link

Member

nicolas-grekas commentedAug 11, 2016

👍
Status: reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Aug 11, 2016

Copy link

Member

fabpot commentedAug 15, 2016

Thank you @Ener-Getick.

fabpot added a commit that referenced this pull request

Aug 15, 2016

bug#19437[PropertyInfo] Fix an error in PropertyInfoCacheExtractor …

ac528c7

…(Ener-Getick)This PR was squashed before being merged into the 3.1 branch (closes#19437).Discussion----------[PropertyInfo] Fix an error in PropertyInfoCacheExtractor| Q             | A| ------------- | ---| Branch?       | 3.1| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |An argument was forgotten in the ``PropertyInfoCacheExtractor`` class leading to exceptions such as```PHP Warning:  ucfirst() expects parameter 1 to be string, array given in /home/guilhem/github/ast-test/vendor/symfony/symfony/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php on line 318```and making it unusable.ping@dunglasCommits-------d19b151 [PropertyInfo] Fix an error in PropertyInfoCacheExtractor

fabpot closed this

Aug 15, 2016

GuilhemN deleted the patch-3 branch

August 22, 2016 12:54

fabpot mentioned this pull request

Sep 3, 2016

Release v3.1.4#19838

Merged