Skip to content

Php8#17

Merged
lucatume merged 20 commits intolucatume:masterfrom
kagg-design:php8
Apr 19, 2021
Merged

Php8#17
lucatume merged 20 commits intolucatume:masterfrom
kagg-design:php8

Conversation

@kagg-design
Copy link
Copy Markdown
Contributor

@kagg-design kagg-design commented Dec 3, 2020

Code and tests work on PHP 7.2-8.0.
Relevant PHPUnit 8.5-9.4 is used by the Composer.
Github actions implemented, as they have PHP 8.0 already.
Tests on Github actions run on ubuntu and windows.
All tests are OK: https://github.com/kagg-design/function-mocker/actions/runs/398379092

lucatume and others added 10 commits October 23, 2020 13:42
…on Windows.

Otherwise, dispatchDynamic() in patchwork cannot locate non-existing functions and replace() returns null instead of expected value.
We cannot run tests on 8.0 so far, as phpunit8+ require that "setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void".

All tests should be edited accordingly.
No backward compatibility with php 5.6 - 7.1.

Should be tagged as the next version of function-mocker: 2.0.0.
"phpunit/phpunit": "8.5 - 9.4" in composer.json makes it automatically.

On php 7.2: phpunit 8.5
On php 7.3 - 8.0: phpunit 9.4
@lucatume
Copy link
Copy Markdown
Owner

lucatume commented Dec 3, 2020

@kagg-design thanks again for the PR; but I would not like to drop PHP 5.6 compatibility if it can be avoided.

@lucatume
Copy link
Copy Markdown
Owner

lucatume commented Dec 3, 2020

I will review this, after the other, and see what can be done.

@kagg-design
Copy link
Copy Markdown
Contributor Author

I do see how compatibility with PHP 5.6 can be maintained. You can find in the PR how many changes new PHPUnit 8+ libraries require. On the surface, declaration of setUp(): void is required, what breaks compatibility with PHP 5.6 immediately.

Some classes have renamed and moved in PHPUnit 8+, so in some methods, I have replaced class names in the argument type declarations. For instance, in \tad\FunctionMocker\MockWrapper::getWrappedInstance

PHPUnit_Framework_MockObject_MockObject -> PHPUnit\Framework\MockObject\MockObject \PHPUnit_Framework_MockObject_Matcher_InvokedRecorder -> PHPUnit\Framework\MockObject\Rule\InvocationOrder

So now method signature looks like

protected function getWrappedInstance(MockObject $object, ExtenderInterface $extender, InvocationOrder $invokedRecorder = null, ReplacementRequest $request = null)

We cannot specify two class names in the signature to satisfy versions of PHPUnit before and after PHPUnit 8. And this is the reason why we cannot be compatible with PHP 7.0 and 7.1, too.

getInvocation() method was removed from PHPUnit 8+, that is why I have added \tad\FunctionMocker\Call\Verifier\InstanceMethodCallVerifier::getInvocations.

My idea is to make the next version of Function Mocker (2.0.0) for PHP 7.2-8.0, and continue support of 1.3.x for PHP 5.6-7.1. It is quite a common solution.

@lucatume
Copy link
Copy Markdown
Owner

lucatume commented Dec 4, 2020

In wp-browser I've faced a similar issue quite often, and there could be a solution that will not require dropping support for PHP 5.6 to support newer versions; I would like to take some time to look into this and see if I can avoid releasing a breaking change.

Ideally, I would like breaking changes to be based on features; I'm not discarding the possibility upfront but would like to evaluate.

Aside from this: I would like to clarify I really appreciate your contribution, and I will look into this next week.

Thanks, have a little more patience.

@kagg-design
Copy link
Copy Markdown
Contributor Author

Thank you @lucatume for your response and your excellent library. I use it widely in my plugins and implemented it in the development of WPML (1 million+ installs). Let us find a proper solution together, I am ready to cooperate further to make Function Mocker compatible with PHP8.

@lucatume
Copy link
Copy Markdown
Owner

lucatume commented Dec 4, 2020

Thank you @lucatume for your response and your excellent library. I use it widely in my plugins and implemented it in the development of WPML (1 million+ installs). Let us find a proper solution together, I am ready to cooperate further to make Function Mocker compatible with PHP8.

I'm sure we can kludge something together; if not, the option to just release a breaking version remains open.

Thanks for all the appreciation.

@kagg-design
Copy link
Copy Markdown
Contributor Author

I have found a way how to run tests on PHP 5.6-8.0: https://github.com/kagg-design/woof-by-category/blob/eb79cf03a9cbb0f8a9ea0b8eaa8efe8d09ea02b2/.make/update-phpunit.sh#L67

Tests are OK now https://github.com/kagg-design/woof-by-category/actions/runs/400753580, but the plugin is far simpler than your library.

If in the the method signature we can write (keeping PHPDoc reasonable)

protected function getWrappedInstance(object $object, ExtenderInterface $extender, object $invokedRecorder = null, ReplacementRequest $request = null)

instead of

protected function getWrappedInstance(MockObject $object, ExtenderInterface $extender, InvocationOrder $invokedRecorder = null, ReplacementRequest $request = null)

it could work then. If you are agree with the above, I can try to update the php8 branch.

Thank you.

@lucatume
Copy link
Copy Markdown
Owner

lucatume commented Dec 7, 2020

I have found a way how to run tests on PHP 5.6-8.0: https://github.com/kagg-design/woof-by-category/blob/eb79cf03a9cbb0f8a9ea0b8eaa8efe8d09ea02b2/.make/update-phpunit.sh#L67

Tests are OK now https://github.com/kagg-design/woof-by-category/actions/runs/400753580, but the plugin is far simpler than your library.

If in the the method signature we can write (keeping PHPDoc reasonable)

protected function getWrappedInstance(object $object, ExtenderInterface $extender, object $invokedRecorder = null, ReplacementRequest $request = null)

instead of

protected function getWrappedInstance(MockObject $object, ExtenderInterface $extender, InvocationOrder $invokedRecorder = null, ReplacementRequest $request = null)

it could work then. If you are agree with the above, I can try to update the php8 branch.

Thank you.

I think this is a reasonable compromise and one that will not impact a user-facing API, all fine by me.

Thanks.

@lucatume
Copy link
Copy Markdown
Owner

@kagg-design I did not forget about this PR, at all.

I'm just working on other stuff; is this holding any of your work? Blocking something?

@kagg-design
Copy link
Copy Markdown
Contributor Author

@lucatume no, not holding anything. I made my work using dev branches on my fork.

I hope to return to my PR today and will try to improve it removing limitations on PHP version...

@lucatume
Copy link
Copy Markdown
Owner

@lucatume no, not holding anything. I made my work using dev branches on my fork.

Ok, this gives me some peace of mind. I will move forward with this when some other OS work is wrapped.

@kagg-design
Copy link
Copy Markdown
Contributor Author

I have added several commits to the PR. Tests are fine with PHP 5.6-8.0 on Linux and Windows: https://github.com/kagg-design/function-mocker/actions/runs/514955873

@kagg-design
Copy link
Copy Markdown
Contributor Author

@lucatume Could you review this PR? I think it would be helpful for many users of Function Mocker library.

Now code is compatible with PHP 5.6-8.0, and tests are OK on Linux and Windows with PHP 5.6-8.0.

@lucatume
Copy link
Copy Markdown
Owner

@kagg-design sorry, I lost track of this.

I will review this and merge it when I get a chance.

@lucatume lucatume mentioned this pull request Apr 19, 2021
lucatume added a commit that referenced this pull request Apr 19, 2021
@lucatume lucatume merged commit b3ea9d4 into lucatume:master Apr 19, 2021
@lucatume lucatume mentioned this pull request Apr 19, 2021
@lucatume
Copy link
Copy Markdown
Owner

@kagg-design thanks again for your work and contribution: much appreciated.

Again: sorry for the lapse.

@kagg-design
Copy link
Copy Markdown
Contributor Author

Thank you for accepting my 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

Successfully merging this pull request may close these issues.

2 participants