[8.x] fix doc blocks #35675

rodrigopedra · 2020-12-19T22:34:47Z

Addresses errors for 8.x branch listed on #35673 .

EDIT: I also made some other fixes I found while assessing doctum error messages.

To clarify, I didn't run doctum myself. I based my code review on the error messages listed on #35673

rodrigopedra · 2020-12-19T22:36:24Z

src/Illuminate/Mail/PendingMail.php

@@ -114,12 +114,11 @@ public function bcc($users)
     * Send a new mailable message instance.
     *
     * @param  \Illuminate\Contracts\Mail\Mailable  $mailable
-     *
-     * @return mixed
+     * @return void


Here I removed the extra blank line between parameters and return, and also changed the return to match the interface.

Mailer implementation send method does not return anything.

Oh okay, did Doctum say something about that ?

No, but when I was reviewing the others I found this. Neither doctum or StyleCI complained about this one.

I will update my opening comment to clarify I made other fixes.

phpstan would have got it I think ;p

rodrigopedra · 2020-12-19T22:36:52Z

src/Illuminate/Notifications/NotificationSender.php

@@ -171,7 +171,7 @@ protected function shouldSendNotification($notifiable, $notification, $channel)
     * Queue the given notification instances.
     *
     * @param  mixed  $notifiables
-     * @param  array[\Illuminate\Notifications\Channels\Notification]  $notification
+     * @param  \Illuminate\Notifications\Notification  $notification


The implementation does not expects an array on this parameter

Hi @williamdes

Maybe this one you want to take a look on how doctum is handling, the error message was odd for this line:

ERROR: The "r" @param tag variable name is wrong (should be "notification") on "Illuminate\Notifications\NotificationSender::queueNotification" in build/doctum/laravel/src/Illuminate/Notifications/NotificationSender.php:177

As you can see the @param variable was not called r. Maybe it is because it used this odd syntax with brackets

Ha, thanks for pointing this out
I had some chance this time because you looked to the old errors report, please have a look to the "5.3.0-dev" one instead :)

Now I see, thanks for the heads up

rodrigopedra · 2020-12-19T22:37:20Z

src/Illuminate/Support/Testing/Fakes/MailFake.php

@@ -347,7 +347,7 @@ public function send($view, array $data = [], $callback = null)
    /**
     * Queue a new e-mail message for sending.
     *
-     * @param  string|array  $view
+     * @param  \Illuminate\Contracts\Mail\Mailable|string|array  $view


Changed to match its interface

rodrigopedra · 2020-12-19T22:37:56Z

src/Illuminate/Support/Testing/Fakes/PendingMailFake.php

-     * @param  \Illuminate\Contracts\Mail\Mailable  $mailable;
-     * @return mixed
+     * @param  \Illuminate\Contracts\Mail\Mailable  $mailable
+     * @return void


Here I removed the extra semicolon, and also changed the return to match the interface.

Mailer implementation send method does not return anything.

rodrigopedra · 2020-12-19T22:42:32Z

From the errors for the 8.x branch the only I left unaddressed was this:

ERROR: "0" @param tags are expected but only "1" found on "Illuminate\Http\Client\Response::throw" in build/doctum/laravel/src/Illuminate/Http/Client/Response.php:236

This was a feature added by commit d034e2c to allow a callback but avoid a breaking change on changing the method's signature.

Maybe the method signature could be changed for 9.x

williamdes · 2020-12-19T22:45:04Z

Version 8.x
------------
  Parsing   done

ERROR: The method "cache" has "1" @param tags but only "0" where expected. on "\cache" in /mnt/Dev/@code-lts/@doctum-fork/laravel.com-next/build/doctum/laravel/src/Illuminate/Foundation/helpers.php:233
ERROR: The "args" parameter of the method "event" is missing a @param tag on "\event" in /mnt/Dev/@code-lts/@doctum-fork/laravel.com-next/build/doctum/laravel/src/Illuminate/Foundation/helpers.php:430
ERROR: The method "event" has "3" @param tags but only "1" where expected. on "\event" in /mnt/Dev/@code-lts/@doctum-fork/laravel.com-next/build/doctum/laravel/src/Illuminate/Foundation/helpers.php:430
ERROR: The method "throw" has "1" @param tags but only "0" where expected. on "Illuminate\Http\Client\Response::throw" in /mnt/Dev/@code-lts/@doctum-fork/laravel.com-next/build/doctum/laravel/src/Illuminate/Http/Client/Response.php:236

 988/988 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 16 secs
  Rendering                                             Rendering done

 1123/1123 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 23 secs

Here are the errors that are not fixed, thank you for your quick work on the other ones !

williamdes · 2020-12-19T22:46:56Z

Seems to be a Doctum 5.3.0-dev bug from the source code, I see no reason to remove the phpdoc tag (from an external view)

Nonsense myself .. 😄

At

framework/src/Illuminate/Http/Client/Response.php

Line 231 in 13462f6

* @param \Closure|null $callback

it needs to be removed ^^

Seems like https://stackoverflow.com/questions/14513356/phpdoc-documenting-a-function-with-a-variable-number-of-arguments will be usefull for the event helper function

rodrigopedra · 2020-12-19T23:04:39Z

Hi @williamdes , thanks for the quick review.

Besides the one for Response@throw, the additional errors weren't in the original #35673 .

I will wait for the maintainers to take a look on those first as to me it is tricky and could be a matter of opinion.

The Response@throw documents a supported feature added to avoid a BC
The cache helper one uses a @param dynamic syntax that is strange to me, but can be there for a reason
The event helper docblock matches the Illuminate\Contracts\Events\Dispatcher@dispatch docblock, therefore I think we could copy the method signature and delegate that. But if someone rebound the events string in the container to something else it could lead to a BC. So I will leave the decision to the maintainers for now.

GrahamCampbell · 2020-12-19T23:10:34Z

We are not changing the mailer stuff yet. We marked it as void, however some people are still using the return value, and we have no good way of providing an alternative for their use case yet, so that's how we arrived having a void contract, but mixed in reality. The mailer component may be getting some refactoring in 9.x if we decide to switch to symfony mailer. The issue with the void-mixed stuff may be resolved properly, at that point.

williamdes · 2020-12-19T23:11:36Z

Hi @GrahamCampbell
So to fix #35673 a PR with only phpdoc fixes is needed ?

GrahamCampbell · 2020-12-19T23:11:42Z

The cache and validation fixes look good, though. Please submit a PR fixing that on 6.x (if the code exists on 6.x, otherwise 8.x is fine).

williamdes · 2020-12-19T23:12:10Z

And for 5.x issues ?

rodrigopedra · 2020-12-19T23:26:31Z

Hey @GrahamCampbell , thanks for your review

Wouldn't reverting the return for Illuminate\Mail\PendingMail@send make it good? The other fixes on the mailer docblocks are legitimate in my opinion (removing an extra semicolon and fixing setGlobalToAndRemoveCcAndBcc to conform with interface)

If you think so I just pushed a commit to revert the send return to my fork.

williamdes · 2020-12-20T11:22:59Z

Hey @GrahamCampbell , thanks for your review

Wouldn't reverting the return for Illuminate\Mail\PendingMail@send make it good? The other fixes on the mailer docblocks are legitimate in my opinion (removing an extra semicolon and fixing setGlobalToAndRemoveCcAndBcc to conform with interface)

If you think so I just pushed a commit to revert the send return to my fork.

I think you can re-open a pull-request :)

taylorotwell · 2020-12-20T22:37:23Z

@rodrigopedra re-opened. Is this ready to go?

GrahamCampbell · 2020-12-20T22:58:07Z

Some of these need fixing on the 6.x series, first.

rodrigopedra · 2020-12-20T23:00:00Z

Hi @taylorotwell I just force-pushed again to force re-syncing the PR.

So now there are just docblock changes. No code changes. I think it is good to go.

Again, I left the Illuminate\Http\Client\Response@throw as it is now, inconsistent but documenting a supported dynamic parameter.

I also didn't change the cache and event helpers docblocks.

I think the event one could be changed, please refer to comment #35675 (comment) for more details.

I can send the needed fixes for 6.x too.

rodrigopedra · 2020-12-20T23:00:51Z

If you want I can make the changes from comment #35675 (comment) before merging.

williamdes · 2020-12-20T23:03:00Z

By the way I just want to say that the variadic is not well documented
Sorry for the spamming but it is important to say

Please refer to 20Tauri/DoxyDoxygen#135 (comment)

fix doc blocks

0d2d047

rodrigopedra commented Dec 19, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

GrahamCampbell closed this Dec 19, 2020

taylorotwell reopened this Dec 20, 2020

revert send docblock to mixed

23ad9e3

williamdes approved these changes Dec 20, 2020

View reviewed changes

taylorotwell merged commit d30dbeb into laravel:8.x Dec 21, 2020

rodrigopedra mentioned this pull request Dec 21, 2020

[6.x] fix doc blocks #35690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[8.x] fix doc blocks #35675

[8.x] fix doc blocks #35675

rodrigopedra commented Dec 19, 2020 •

edited

rodrigopedra Dec 19, 2020

williamdes Dec 19, 2020

rodrigopedra Dec 19, 2020

williamdes Dec 19, 2020

rodrigopedra Dec 19, 2020

rodrigopedra Dec 19, 2020

williamdes Dec 19, 2020

rodrigopedra Dec 19, 2020

rodrigopedra Dec 19, 2020

rodrigopedra Dec 19, 2020

rodrigopedra commented Dec 19, 2020

This comment has been minimized.

williamdes commented Dec 19, 2020

williamdes commented Dec 19, 2020 •

edited

rodrigopedra commented Dec 19, 2020

GrahamCampbell commented Dec 19, 2020

williamdes commented Dec 19, 2020

GrahamCampbell commented Dec 19, 2020

williamdes commented Dec 19, 2020

rodrigopedra commented Dec 19, 2020

williamdes commented Dec 20, 2020

taylorotwell commented Dec 20, 2020

GrahamCampbell commented Dec 20, 2020

rodrigopedra commented Dec 20, 2020

rodrigopedra commented Dec 20, 2020

williamdes commented Dec 20, 2020

[8.x] fix doc blocks #35675

[8.x] fix doc blocks #35675

Conversation

rodrigopedra commented Dec 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigopedra commented Dec 19, 2020

This comment has been minimized.

williamdes commented Dec 19, 2020

williamdes commented Dec 19, 2020 • edited

rodrigopedra commented Dec 19, 2020

GrahamCampbell commented Dec 19, 2020

williamdes commented Dec 19, 2020

GrahamCampbell commented Dec 19, 2020

williamdes commented Dec 19, 2020

rodrigopedra commented Dec 19, 2020

williamdes commented Dec 20, 2020

taylorotwell commented Dec 20, 2020

GrahamCampbell commented Dec 20, 2020

rodrigopedra commented Dec 20, 2020

rodrigopedra commented Dec 20, 2020

williamdes commented Dec 20, 2020

rodrigopedra commented Dec 19, 2020 •

edited

williamdes commented Dec 19, 2020 •

edited