Implement a Result type in Drupal Core

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.

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

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...

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.

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.

I made some more comments and others still need to be resolved.

I think this looks good, the documentation here looks great and the code itself looks clear.

I think we should add a followup issue for api.d.o to handle `@template` in a good way.

Additionally, we don't usually add new functionality without implementing it somewhere, and we can't really add this to any existing api's without breaking backwards compatibility. I wonder where/how we're going to implement this, is there a followup already created where we know where can implement this?

AFAIK the implementation is to be handled at #3425212: [PP-1] Migrate BigPipe and Renderer fiber implementation to use Revolt Event Loop

Is there a compelling reason to use a Drupal-specific implementation rather than adopting an existing solution? I’ve noticed some Composer packages that offer a similar goal.

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.