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.

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

Support bypass readonly in PHP 82 #39

Closed
Myks92 opened this issue May 14, 2023 · 17 comments
Closed

Support bypass readonly in PHP 82 #39

Myks92 opened this issue May 14, 2023 · 17 comments

Comments

@Myks92
Copy link

Myks92 commented May 14, 2023

In PHP 82, it is now possible to mark classes "readonly". However, tests of such classes give an error: "Class "Password Hasher" is declared "readonly" and cannot be duplicated". I would like this library to be able to fix these errors as well.

Fixtures:

final readonly class FinalClass
{
}
@Ilyes512
Copy link

Ilyes512 commented Jul 1, 2023

This would also help mocking final readonly classes, because you get the following error when you try to mock it:

Fatal error: Non-readonly class Mockery_0_SomeClass cannot extend readonly class AnotherClass in /src/vendor/mockery/mockery/library/Mockery/Loader/EvalLoader.php(34) : eval()'d code on line 30

@Myks92
Copy link
Author

Myks92 commented Jul 1, 2023

@dg! I found a library that solves this problem: https://github.com/zoltanka/bypass-readonly . Now you can use it, but in the future I would like to have support in the original library. Will you be able to implement it?

@jg-development
Copy link

+1 plz

@jg-development
Copy link

@Myks92 maybe add a pull request .... could make it easier for @dg

@dg
Copy link
Owner

dg commented Aug 23, 2023

The question is, should it remove readonly in the class definition or also for all variables?

@JanTvrdik
Copy link

I think it should be done for all properties as well.

@jg-development
Copy link

hard question .... do we even need this for all variables?
I mean ... I mock a final readonly class .... ok
I mock a final class with readonly properties .... should be filled over constructor + constrcutor of mock? no need for removal

@dg
Copy link
Owner

dg commented Aug 23, 2023

One more thing: should readonly removal be enabled by default? I can't think of any reason it might be a problem, as in the case of final.

@jg-development
Copy link

YES .... thx

@dg dg closed this as completed in 1031e1d Aug 23, 2023
dg added a commit that referenced this issue Aug 23, 2023
@jg-development
Copy link

thx for the quick work ... with the new tag I can mock again

@Ilyes512
Copy link

Oh nice :) Thx @dg!

@dg
Copy link
Owner

dg commented Aug 23, 2023

@jg-development please try this master, if it works I will tag a new version

@jg-development
Copy link

tested and works with master + prophecy

        BypassFinals::enable();
        
        $deliveryFilter = $this->prophesize(DeliveryFilter::class);
        
        $deliveryFilter->__toString()->willReturn('123123');
        $deliveryFilter = $deliveryFilter->reveal();

        $queryString = (string) $deliveryFilter;

dg added a commit that referenced this issue Aug 23, 2023
dg added a commit that referenced this issue Aug 23, 2023
@lucascesconetto
Copy link

@dg One question, for versions like php 8.1, does this also remove readonly from properties?

Example in PHP 8.1:

final class Person {
      public function __construct(
           public readonly string $firstName,
           public readonly string $lastName,
      ) {}
}

@dg
Copy link
Owner

dg commented Aug 31, 2023

Yeah, it removes readonly everywhere in the code.

@lucascesconetto
Copy link

lucascesconetto commented Sep 1, 2023

@dg Is it possible to configure whether or not I want to use this readonly bypass feature ??

First point is that this broke codes, we have libs that check if the properties are readonly based on reflection, and with this feature that you added, it broke these codes, I understand that this should be configurable or at least have entered as a "Major" .

Another reason I see this feature being optional and configurable is that in some cases we want to ensure immutability. And the way it is enabled by default today, it is not possible to have that guarantee.

@andrevitor103
Copy link

@dg Is it possible to configure whether or not I want to use this readonly bypass feature ??

First point is that this broke codes, we have libs that check if the properties are readonly based on reflection, and with this feature that you added, it broke these codes, I understand that this should be configurable or at least have entered as a "Major" .

Another reason I see this feature being optional and configurable is that in some cases we want to ensure immutability. And the way it is enabled by default today, it is not possible to have that guarantee.

Hello everyone, I opened this PR with a suggestion on how to solve this problem. If you can look into it, thank you. [PR]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants