Add Readonly support #623
Add Readonly support #623
Conversation
WalterWoshid
commented
Apr 25, 2024
WalterWoshid
commented
- closes Mocking PHP 8.2 readonly classes generates readonly double with untyped property and fails #586
|
Ran the tests on PHP 8.3 and PHP 7.2, both ran successfully. |
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Prophecy\Doubler\ClassPatch\ProphecySubjectPatch; |
There was a problem hiding this comment.
If this class is part of the final API, it should not be in the ClassPatch namespace at all IMO.
And this class should probably be tagged as @internal
There was a problem hiding this comment.
What defines the final API? Where should I place it instead?
There was a problem hiding this comment.
I understand that ClassPatch is not a good place but I don't really have an awesome idea, so I guess Doubler\Generator is reasonable. (or Doubler\Generated maybe)
| * @return array<string, string> | ||
| * | ||
| * @phpstan-return array<string, 'public'|'private'|'protected'> | ||
| * @return array<string, PropertyNode> |
There was a problem hiding this comment.
This cannot be done as it. This ClassNode is not internal (it is part of the API exposed to custom class patches) so we must preserve backward compatibility. Changing the return type is a BC break.
There was a problem hiding this comment.
It's not possible to break the backward compatibility mostly because of the cross dependency between phpspec et prophecy. But it's also best to not break for user experience as well. (we didn't break for #637 , I don't think we will break here either!)
| if ('__construct' === strtolower($name)) { | ||
| $method->setCode( | ||
| $method->getCode() . | ||
| '$this->objectProphecyClosureContainer = new \Prophecy\Doubler\ClassPatch\ProphecySubjectPatch\ObjectProphecyClosureContainer();' |
There was a problem hiding this comment.
should this be at the beginning in case there's an early return?
|
|
||
| namespace Prophecy\Doubler\Generator\Node; | ||
|
|
||
| class PropertyTypeNode extends TypeNodeAbstract |
There was a problem hiding this comment.
Can you rebase this PR since there's a new type API that should match better your usage? (also it has been a lot of changes so it make sense to rebase your work)
| * @phpstan-param 'public'|'private'|'protected' $visibility | ||
| */ | ||
| public function addProperty($name, $visibility = 'public') | ||
| public function addProperty($name, $visibility = 'public', ?PropertyTypeNode $typeNode = null) |
There was a problem hiding this comment.
I think this method should be public function addProperty(PropertyNode $name)... With an appropriate BC layer and trigger deprecation of course.
| /** | ||
| * @var array<string, PropertyNode> | ||
| */ | ||
| private $propertyNodes = array(); |
There was a problem hiding this comment.
Why not continuing usage of properties property here? (either way, it cannot be both, you need to remove one)
|
|
||
| foreach ($class->getProperties() as $name => $visibility) { | ||
| $code .= sprintf("%s \$%s;\n", $visibility, $name); | ||
| foreach ((array) $class->getPropertyNodes() as $propertyNode) { |
There was a problem hiding this comment.
getPropertyNodes() returns an array anyway, why casting it?
| foreach ($class->getProperties() as $name => $visibility) { | ||
| $code .= sprintf("%s \$%s;\n", $visibility, $name); | ||
| foreach ((array) $class->getPropertyNodes() as $propertyNode) { | ||
| $code .= $this->generateProperty($propertyNode)."\n"; |
There was a problem hiding this comment.
For new Type nodes, we use the __toString() method. I believe that keeping the same generation way between classes impacts the maintainability so it would be better to use a toString() method here as well.
| */ | ||
| public function setVisibility(string $visibility) | ||
| { | ||
| $visibility = strtolower($visibility); |
|
@WalterWoshid is it fine to you if I take your work to make a PR that will be merged? |
|
@Nek- Yes, go ahead :) |
4 participants
