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.

Sign up for GitHub

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

Jump to bottom

Align adapter types to ext reflection collection-ish method types #572

Merged
merged 3 commits into from
May 7, 2020
Merged

Align adapter types to ext reflection collection-ish method types #572

merged 3 commits into from
May 7, 2020

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented May 6, 2020

Fixes #568

Potentially a BC break, but I would rather say that here BetterReflection is not respecting the types inherited from ext-reflection.

@asgrim can I please have your opinion on this? Should I release it under 4.x, or push 5.0.0? I'm inclined to call it a 4.x problem (since we don't control the type)

@dependabot-preview
Bump vimeo/psalm from 3.10.1 to 3.11.2
d47ba33 
Bumps [vimeo/psalm](https://github.com/vimeo/psalm) from 3.10.1 to 3.11.2.
- [Release notes](https://github.com/vimeo/psalm/releases)
- [Commits](vimeo/psalm@3.10.1...3.11.2)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
@Ocramius Ocramius added the bug label May 6, 2020
@Ocramius Ocramius added this to the 4.1.0 milestone May 6, 2020
@Ocramius Ocramius requested a review from asgrim May 6, 2020 18:33
@Ocramius
Align types of reflection adapters collection-ish methods to `ext-ref…
beca868 
…lection` types

Effectively, this ensures following type changes:

 * `ReflectionClass#getProperties() : array<string, ReflectionProperty>` => `ReflectionClass#getProperties() : list<ReflectionProperty>`
 * `ReflectionClass#getReflectionConstants() : array<string, ReflectionClassConstant>` => `ReflectionClass#getReflectionConstants() : list<ReflectionClassConstant>`
 * `ReflectionClass#getTraits() : array<int, ReflectionClass>` => `ReflectionClass#getTraits() : array<trait-string, ReflectionClass>`
 * `ReflectionObject#getProperties() : array<string, ReflectionProperty>` => `ReflectionObject#getProperties() : list<ReflectionProperty>`
 * `ReflectionProperty#setValue() : void|null` => `ReflectionProperty#setValue() : void`
@Ocramius Ocramius force-pushed the fix/align-adapter-types-to-ext-reflection-collection-ish-method-types branch from fcbf996 to beca868 Compare May 6, 2020 18:35
@Ocramius Ocramius modified the milestones: 4.1.0, 4.2.0 May 6, 2020
@Ocramius Ocramius changed the title Fix/align adapter types to ext reflection collection ish method types Align adapter types to ext reflection collection-ish method types May 6, 2020
@asgrim
Copy link
Member

asgrim commented May 7, 2020

@asgrim can I please have your opinion on this? Should I release it under 4.x, or push 5.0.0? I'm inclined to call it a 4.x problem (since we don't control the type)

Yeah, I'd view this as an implementation bug. No idea on whether many people are actually using the adapters, I'd guess not many (since they don't have much real advantage over built-in reflection I guess, beside static reflection), so I figure it probably won't affect (m)any anyway.

@Ocramius
Removed reference to webmozart/assert dependency in sources, since …
8142147 
…we are not requiring it ourselves

All code paths using `webmozart/assert` were mostly type-checks about unhappy paths that should not (in theory)
be possible.

Ref: #572 (comment)
@Ocramius
Copy link
Member Author

Ocramius commented May 7, 2020

Applied patch to get rid of Webmozart\\ in src/ (found another one lurking): will merge once green 👍

@asgrim
Copy link
Member

asgrim commented May 7, 2020

LGTM :shipit:

@Ocramius Ocramius merged commit 23860f1 into master May 7, 2020
@Ocramius Ocramius deleted the fix/align-adapter-types-to-ext-reflection-collection-ish-method-types branch May 7, 2020 16:36
@ondrejmirtes
Copy link
Contributor

No idea on whether many people are actually using the adapters

I'm a heavy user of the adapters so you'll definitely hear from me if you screw something up ❤️ 👍

@ondrejmirtes
Copy link
Contributor

(I hope you see the friendliness in the message, I used a similar tone @Ocramius often uses :))

@Ocramius
Copy link
Member Author

Ocramius commented May 7, 2020

heh, sorry in advance if this breaks something. in theory, if you relied on the ext-reflection API, you should be fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asgrim asgrim asgrim approved these changes

Assignees

@asgrim asgrim

Labels
bug
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ocramius @asgrim @ondrejmirtes