Problem/Motivation

Many tests (7 exactly) cause this deprecation:

Returning a value from a destructor is deprecated

Steps to reproduce

Run phpunit

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3612500

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

graber created an issue. See original summary.

graber opened merge request !16395

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +PHP 8.6
Related issues: +#3608511: [meta] PHP 8.6 support

Or, we could wait for upstream issue PHP 8.6 - Returning a value from a destructor is deprecated to get fixed.

graber’s picture

graber commented
Status: Active » Needs review

From what I saw.. prophecy is removed from phpunit and we're using a bridge.. We'll keep getting such issues until we switch one day.
I think this is a small step forward then.

Not sure why this one fails even if all is green: https://git.drupalcode.org/issue/drupal-3612500/-/jobs/11090784

Moving to review.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@graber I bet this commit will explain https://git.drupalcode.org/project/drupal/-/merge_requests/16220/diffs?c...

joachim’s picture

joachim commented

I don't think replacing prophecy mocking with PHPUnit mocking is the right thing to do here.

It looks like this is getting fixed upstream?

catch’s picture

catch
he/him
Primary language English
commented

While this is failing lint caching in pipelines doesn't work, which with phpstan adds 2-3 minutes to every MR pipeline, which is not the end of the world but also isn't ideal. I don't see a particular problem switching from prophecy to phpunit mocking, we use both in core, although it would be good to avoid the use of #[AllowMockObjectsWithoutExpectations] if we can unless there's a good reason for that.

@joachim the upstream issue was opened three weeks ago by us and only has people from this issue involved in it, so it doesn't particularly look like it will get fixed promptly.

graber’s picture

graber commented

Slightly refactored to remove #[AllowMockObjectsWithoutExpectations].
@joachim

I don't think replacing prophecy mocking with PHPUnit mocking is the right thing to do here.

Just a premonition, or something more?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Looks generally good to me, but we should IMHO start avoiding creating mocks without expectations that will be causing problems with PHPUnit 13+. See comments inline.

graber’s picture

graber commented
Status: Needs work » Needs review

Makes sense, addressed, Re the result caching concern - investigated a bit, replied in the comment, not sure if a follow-up is needed - I believe it's smaller than it seemed.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me. I think we still have a case of a mock w/o expect, but if not fixed here it will be just one to fix in the PHPUnit 13 issue.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Found the expectation for the mock, it's just a few lines down.

I think we should see if this fixes lint caching, so going ahead here. Committed/pushed to main, thanks!

Needs a backport MR for 11.x

  • catch committed cdb890ac on main 
    fix: #3612500 Various unit tests: Returning a value from a destructor is...

catch closed merge request !16395