Skip to content

[PropertyInfo] Fix interface handling in PhpStanTypeHelper #59012

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

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

janedbal
Copy link
Contributor

@janedbal janedbal commented Nov 27, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Previously, deserializing object with Interface<T> property was failing on undefined array key in PhpStanTypeHelper (generic Clazz<T> works just fine):

ErrorException: Undefined array key 0
/app/backend/vendor/symfony/property-info/Util/PhpStanTypeHelper.php:171
/app/backend/vendor/symfony/property-info/Util/PhpStanTypeHelper.php:49
/app/backend/vendor/symfony/property-info/Extractor/PhpStanExtractor.php:130
/app/backend/vendor/symfony/property-info/PropertyInfoExtractor.php:93
/app/backend/vendor/symfony/property-info/PropertyInfoExtractor.php:66
/app/backend/vendor/symfony/property-info/PropertyInfoCacheExtractor.php:102
/app/backend/vendor/symfony/property-info/PropertyInfoCacheExtractor.php:69
/app/backend/vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:669
/app/backend/vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:646
/app/backend/vendor/symfony/serializer/Normalizer/AbstractNormalizer.php:386
/app/backend/vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:243
/app/backend/vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:349
/app/backend/vendor/symfony/serializer/Serializer.php:247
/app/backend/vendor/symfony/serializer/Serializer.php:152

@janedbal
Copy link
Contributor Author

Actually, this fix seems to be valid even for 6.4, should I rebase?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Please submit another PR for 6.4: this will help with merging up.

@OskarStark OskarStark changed the title [PropertyInfo] Fix interface handling in PhpStanTypeHelper [PropertyInfo] Fix interface handling in PhpStanTypeHelper Nov 27, 2024
@janedbal
Copy link
Contributor Author

Please submit another PR for 6.4

Will do that once this is approved.

@xabbuh
Copy link
Member

xabbuh commented Dec 3, 2024

@janedbal Can you open the PR targeting 6.4?

@janedbal
Copy link
Contributor Author

janedbal commented Dec 3, 2024

@xabbuh Sure, but it does not affect mergeability here, right?

@mtarld
Copy link
Contributor

mtarld commented Dec 3, 2024

@janedbal, the 6.4 changes will be upmerged up to 7.3, therefore this 7.1 PR will become useless.

@janedbal
Copy link
Contributor Author

janedbal commented Dec 3, 2024

Looks like we misunderstood each other, so I just rebase it to 6.4. No need for another MR, right?

@xabbuh
Copy link
Member

xabbuh commented Dec 3, 2024

If there is not much difference between the branches, rebasing the PR on 6.4 should be enough. Having a different PR for 7.1 would only be useful if there are conflicts to be expected when merging up which would then serve as a reference for us to ease resolving the conflicts.

@janedbal janedbal force-pushed the property-info-iface-fix branch from a56d730 to 36f4d3f Compare December 3, 2024 11:56
@janedbal janedbal changed the base branch from 7.1 to 6.4 December 3, 2024 11:56
@janedbal
Copy link
Contributor Author

janedbal commented Dec 3, 2024

Rebased and changed base branch to 6.4

@janedbal
Copy link
Contributor Author

janedbal commented Dec 5, 2024

Is this mergeable now? Or are we waiting for code-owner approve? Or?

@chalasr chalasr force-pushed the property-info-iface-fix branch from 36f4d3f to 88e7a72 Compare December 10, 2024 11:00
@chalasr
Copy link
Member

chalasr commented Dec 10, 2024

Thank you @janedbal.

@chalasr chalasr merged commit c439a58 into symfony:6.4 Dec 10, 2024
7 of 11 checks passed
mtarld added a commit to mtarld/symfony that referenced this pull request Dec 10, 2024
@janedbal
Copy link
Contributor Author

Thank you!

fabpot pushed a commit to mtarld/symfony that referenced this pull request Dec 11, 2024
fabpot added a commit that referenced this pull request Dec 11, 2024
…Helper (mtarld)

This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] [7.1] Fix interface handling in PhpStanTypeHelper

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

Upmerge #59012 up to 7.1

Commits
-------

65b370e [PropertyInfo] Upmerge #59012
xabbuh added a commit that referenced this pull request Dec 11, 2024
* 7.1:
  Fix resolve enum in string type resolver
  [PropertyInfo] Upmerge #59012
  [BeanstalkMessenger] Round delay to an integer to avoid deprecation warning
  [PropertyInfo] Fix interface handling in `PhpStanTypeHelper`
  [HttpClient] Test POST to GET redirects
  [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data)
  fix: preserve and nowrap in profiler code highlighting
xabbuh added a commit that referenced this pull request Dec 11, 2024
* 7.2:
  Bump Symfony version to 7.2.2
  Fix resolve enum in string type resolver
  Update VERSION for 7.2.1
  Update CHANGELOG for 7.2.1
  [PropertyInfo] Upmerge #59012
  [BeanstalkMessenger] Round delay to an integer to avoid deprecation warning
  [Console] Fix tests
  [PropertyInfo] Fix interface handling in `PhpStanTypeHelper`
  [TypeInfo] Remove useless dependency
  [HttpClient] Test POST to GET redirects
  [TypeInfo] Fix outdated README
  [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data)
  [TypeInfo] Fix handle nullable with mixed
  fix: preserve and nowrap in profiler code highlighting
This was referenced Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants