Docblock and Strict Code updates

[Artistan]

Updates for...

• facades methods
• static methods via __callStatic
• return null when mixed not void (strict)
• return void - removed return
• added @throws to to a few methods
• added inline docblocks to allow strict checks where necessary

[ConnorVG]

I see a whole lot of no here…

[Artistan]

feedback would be helpful. thank you.

[tillkruss]

You've touched 36 files in on PR and the changes are not really related. I'd suggest to submit several small PRs.

I don't think Taylor will merge inline docblock or things like return null;, but some of the docblocks might be helpful.

[Artistan]

Ya, it is a lot, I finally got it to a point of valid code inspections using PhpStorm
More than willing to break it down, but I would rather get this feedback before I proceed...

I believe that the return null; is a valid strict return case.

[ConnorVG]

There are a lot of things wrong with this, here are a few:

1. https://github.com/laravel/framework/pull/21517/files#diff-91e6cec6b080a99e66b5dd56fa5c3bd3R9 – interfaces don't give a shit about their implementations. There is no point docblocking a method (@method getJobId()) that doesn't exist on the interface;
2. https://github.com/laravel/framework/pull/21517/files#diff-a18aa0065696398ebfc40891576da288R62 – in Laravel's source, we never inline docblock a variable like this (that I know of). We don't write docblocks for each specific editor to give you fancy abilities (we use the generic docblock standards such as: @param and @return), we docblock things so we know what they are and what they're doing if they cannot be immediately deduced;
3. return null; === return; === not returning at the end of a function;
4. https://github.com/laravel/framework/pull/21517/files#diff-a18aa0065696398ebfc40891576da288R210 – you remove readability for seemingly no reason;
5. https://github.com/laravel/framework/pull/21517/files#diff-a18aa0065696398ebfc40891576da288R251 – even though I've already mentioned this in 2, I want to just say that it's really bizarre inline docblocking a variable that isn't immediately used in the next line;
6. https://github.com/laravel/framework/pull/21517/files#diff-4e07c2198357095a33f2336b6fae43afR16 – this one is really bizarre, leave it to the implementation to be documented. There is no point trying to achieve DRY code when we have such sporadic documentation duplication;
7. https://github.com/laravel/framework/pull/21517/files#diff-ea6a3c4bc6dbcbe5d44e929950475632R6 – these exist purely to display their availability, their @description should not be duplicated here (you do this about a million and seven times).

My point is simple: if you want to contribute to the framework, feel free to. Though, don't expect all your preferences to be adopted just because you like it that way.

That said, this is unacceptable for a pull request:

Pull requests should be focused, tested and standardised. This feels a lot like you just wanted to get loads of commits into a big repository so when you apply for a job you can be like "Ooh, look how much I've contributed!". Not acceptable.

[Artistan]

1. is a docblock / quick fix for IDEs that is not a breaking change unlike the accepted pull request into master
   https://github.com/laravel/framework/pull/21303/files

2. Defining $this inline is the only way to define $this for a trait (in this case it is always a model, so why not document that) ... this is useful for code inspection / validation.
   

3. return null; === return; === not returning at the end of a function;
   .. useful for proper code inspection / validation
   

4. this is useful for proper code inspection / validation
   

5. Defining $this and $through inline is the only way to define them for a trait
   ... this is useful for code inspection / validation.

6. this is for proper ide definition of static magic methods
   

7. point taken, keep it simple.

lastly. I appreciate your feedback. I have contributed many times and plan to continue. Please feel free to be as brutal as you wish. Any feedback is good feedback.

P.S. - I have a job. thanks.

[ConnorVG]

The "breaking change" comment is about a PR that was merged into master that didn't fail any tests. Have you got any information for me to find out about what it broke and how it was unreasonable?

Though, don't expect all your preferences to be adopted just because you like it that way.

This is my point. Your direction with this is "proper code inspection / validation", which is invalid. Your true point is "better support PHPStorm's inspection / integration" (validation is nonsense here…). Your point "this is for proper ide definition of static magic methods" again boils down to: this is a lot of repeated documentation that you want specifically for your IDE.

If this was to happen, which I'm willing to bet won't, it would need to be done everywhere. Not just these seemingly random cherry picked files.

[Artistan]

the "breaking change" is mentioned here
#21298 (comment)

These docblocks are the most common methods used by many people in a static context. That is why have been previously defined in some ide-helpers. Many of these definitions already exist in the framework. They improve the readability in the code rather than having to trace magic methods. IDE support and validation is just a benefit. This helps the community as a whole to adapt more of the functionality of the framework. Eventually I would love to help to get it done everywhere. That is taking me a lot of time, hence the large request... I will break it down.

Here are some other magic method and mixin requests that I have done ... as a frame of reference
#21296
#20229
#20224

thanks again.

[ConnorVG]

I assume you see the difference and are just trying to avoid it? This PR has 23 commits and does a lot more than one thing. You also do completely different things to those other PRs (you change non-doc code blocks, you add duplicate descriptions, you describe non-firstclass methods on interfaces).

Just fyi too, not being a brute, you asked for feedback so I gave you some 😄

[Artistan]

As I said, all feedback is good feedback. I appreciate you taking the time to give me yours.
I will break down the changes and clean them based on your feedback and submit smaller updates for approval and/or additional feedback.

Cheers.