Implement a Result type in Drupal Core

Added a merge request to show the proposed implementation. However this "Needs Work" because we can't quite tell PHPStan what we want to do :)

However, this allows async issues to start using the type and demonstrate its usefulness.

I've asked for some help in the PHPStan discussions on GitHub and Ondrej was very kind in providing more insights: https://github.com/phpstan/phpstan/discussions/10667. It turns out I was running into a known issue https://github.com/phpstan/phpstan/issues/6732.

I've now pushed a workaround using a type annotation :)

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This was failing on some coding standards (parts of which is PHPCS not yet understanding more complex PHPStan types). These were fixed in a commit amend.

I missed Catch's feedback on the missing bool return types for isOk/isError which I've now added :D

It looks straightforward so not sure a test needed.

But it needs CR with example of use to announce it to module developers and maybe follow-up to apply monadic result to some core places

I wasn’t quite sure how to test it. I suppose we could test that the isOk and isError types do what they say.

What I’ve previously done is write a few function calls around it with PHPStan at level 9 to make sure all the type annotations had the right behavior. We don’t really have infrastructure for that though and that should be solidified when we adopt it :)

See the linked PHPStan sandbox for that bit of code

I’ll write a CR proposal in the coming days unless someone is faster

I've applied the suggested changes to the documentation and removed the sentence about the list example, instead showing it as code which I hope helps it more easily click with developers.

Nice!

Shouldn't we have test coverage though to make sure this does what is expected?

I suppose we could test that the isOk and isError types do what they say.

Yes, the tests don't have to be large or complex for just a few types. Like you say, it's just to test the types do what's advertised, and also to provide the coverage. They can always be expanded for edge cases as needed.

I'll crank out a test this week, and will make sure PHPStan returns accurate results (part of the tests).

Here's my initial contribution, without any unit tests: https://phpstan.org/r/3964689e-925b-4b7f-bdc5-eca6d4eb7cf5

• There's two class-level generics, OkT and IdentityValue
• Generic hints are always lost in a static context, so both static ok and error methods have to define their own local method-level generics
• PHPStan reports dumped type is Drupal\Core\Result<bool, int>, which means the type hint of the generic IdentityValue is preseved.
• The OkT generic is locked down to a bool both at the class template and the class property, all compilation warnings have been resolved

There is some funny business with having to specify:

    /** @var OkT $ok */
    $ok = false;
    return new self($ok, $value);

But that is what was required to resolve the Method Drupal\Core\Result::ok() should return Drupal\Core\Result<T, never> but returns Drupal\Core\Result<T, T> compile error in https://phpstan.org/r/e9c7213e-b7ad-47f8-93b4-ec19e479b688

Might want to ask Ondrej or one of the smarter guys about that one.

Now about those unit tests...where to place them...

Sorry Alexander Allen! Looks like you started off from an older starting point. From a types perspective the MR is already ready to go.

I've updated the issue summary's remaining tasks to better reflect that.

Haha @Kingdutch, I played myself there! I'm still laughing over here.

I rebased 11.x so it's nice and up to date and I'm going to take a crack at writing a basic test that checks for the class instance, does some basic returns, etc. just too see if I can help a bit. Cheers!

Ran the test-only feature to verify/show the test coverage is there https://github.com/phpstan/phpstan/issues/6732 which it is!

Reading the CR, believe all the info is there with examples. Added the default branch of 11.x

Appears all feedback/threads have been addressed.

Code review not entirely sure don't see other examples in core of using things like @template T or @param T $value

I'm a +1 but going to leave in review for a better code review.

Going to take a leap of faith and mark it

I read the MR and made comments there. This needs work to make sure that the result on a.d.o is readable. This is using tags that a.d.o does not understand and the output is not legible.

Moving to needs work for #19.

I am not sure I can address all the review issues by myself, but I will start by looking into the readability issues created by using generic tags.

Thank you for the review @quietone.

@alexander allen, the short answer is that I don't know much about how api.drupal.org works. I figure out it by testing on my local version. The API project page → has a link to instructions for setting it up locally.

Having said that, if we use the tags that are documented in the Coding Standards for comments → , it will work much better. I was only able to make the suggestions I did after testing locally.

Thanks for the additions and questions AlexanderAllen and quietone.

I've updated the comments throughout the file to address the feedback, after setting up api.drupal.org locally (that was more difficult than I expected). I wasn't familiar with the tooling so I appreciate the link! There were a few hurdles (such as the nested multi-line comment) but I've done my best to adjust the docblock so that it renders nicely on api.drupal.org and also added a link to explain the generics. I've ensured there's a newline between template definitions so that they at least don't bundle into a single paragraph but remain readable.

I realize that Drupal hasn't really used generics yet, but they're a very powerful feature to help developers deal with "container classes". In this case as AlexanderAllen explained, they're indeed not optional, so removing them to improve the documentation rendering would be a downgrade for anyone using a static analysis tool.

As an example the PHPStan types will make the following type detection possible and help you handle both the success and error path:

/**
  * @return Result<string, Exception>
  */
function get_result() : Result {
}

$result = get_result();

$value = $result->getValue(); // PHPStan will tell you that `$value` is `string|Exception`
if ($result->isOk()) {
  $value = $result->getValue(); // PHPStan will know `$value` is `string`
}
else {
  $value = $result->getValue(); // PHPStan will know `$value` is `Exception`
}

It's a really powerful feature and I hope we can improve the API Doc support for it in the future. Especially with applications such as 🌱 Static analysis for EntityStorage Active coming up to better document entity storage classes (and remove a bunch of logic that's currently in mglaman/phpstan-drupal).

I've done my best to change the way it's documented in the class to ensure that it remains readable within the current capabilities of api.drupal.org.

I think if the phpstan template usage is additional and not replacing other docs, and genuinely helps with phpstan, as is the case here, then it should be fine to add it.

There was another issue recently, which I can't find right now but will link when/if I see it again, which was using templates more as a shorthand to prevent copypasta, and that seems like a net-loss for those of us who don't use IDEs.