Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Added "declare(strict_types=1);" to stubs #50349

Closed
wants to merge 1 commit into from

Conversation

ash-jc-allen
Copy link
Contributor

@ash-jc-allen ash-jc-allen commented Mar 2, 2024

Hey! This PR is a little different, but it proposes adding declare(strict_types=1); to the top of all the stubs used for generating files.

For off, apologies for the long PR description haha!

Just for some context, I'm a freelance web dev and I'm usually brought in as an extra pair of hands to work on existing projects. My first job is always to clean up existing bugs that the other devs haven't had time to get to. A huge number of bugs that I've fixed would probably never have occurred in the first place if return types, type hints and strict type-checking were being used.

After speaking to other devs, I found that they didn't tend to use strict type-checking because they didn't actually know it was a thing. But after explaining the benefits, I see it being used more often in their code.

So my idea behind this PR is that all new files would have strict type-checking automatically enabled for the developer. For developers who are already aware of strict type-checking and are using it, it'll probably just save a few key presses.

But I feel like it could have a huge impact on developers that don't know what it is. I imagine for some, they'll think "Hmmmm, what's this?" and they'll Google it and find something new. And for those that don't question it, I guess we're silently helping them to reduce the chance of any type-related bugs.

My main goal for proposing this is that it can be used to help educate developers. I learned a huge amount from the Laravel conventions when I was starting out in web dev. And I didn't learn about strict type-checking for a while. But if this had been part of Laravel at the time, I'd have learned about it a lot sooner and probably would have avoided a lot of bugs.

Of course, I know the goal of the stubs is to be simplistic and that a dev can publish them and make changes if they want to add this themselves (this is something I usually do). But I think this could be a nice way of promoting and encouraging using PHP's type system to reduce bugs.

If you don't think this will be useful, or think it might cause too much confusion for devs, I can totally see where you're coming from! 😄

@devajmeireles
Copy link
Contributor

I don't think that should be a concern for Laravel. Those who know what this is for will look to add to their files or edit the stubs of their projects.

@ash-jc-allen
Copy link
Contributor Author

That's a fair and valid point!

As I say, I don't think it's going to do much for developers who already know about this, apart from saving a few key presses.

But it could be a good way of educating and encouraging developers who don't know about it to start using it. I think it's a nice way of introducing the concept and instilling good practices, and could start off some good habits for new/junior developers too that are just starting out 🙂

@tomschlick
Copy link
Contributor

I think this is a great change and is something I do on every project I start.

However I would target Laravel 11 so it is clearer that it potentially could be a breaking change for newly generated files if someone was doing some funky type juggling.

@nunomaduro
Copy link
Member

I think if we were ever to make a change like this, we would have to analyze thoroughly this change across the entire ecosystem — mostly to detect possible breaking changes associated with this change. At this time, there are hundreds of code snippets we tell users they can execute in their controllers/models, etc., and those snippets have only been tested outside the strict_types mode.

In addition, just like many style changes we've made in the past, such as adding types to all user-land code, if we were to add declare(strict_types=1); to stubs, we would have to add them to all files in user-land code, skeletons, stubs, documentation, starter kits, etc — mostly to keep the typing style consistent. And, test those files under the 'strict_types' mode to ensure everything keeps working as expected.

@MrPunyapal
Copy link
Contributor

I don't think that should be a concern for Laravel. Those who know what this is for will look to add to their files or edit the stubs of their projects.

And also we can add a declare(strict_types=1); into all files with a single command vendor/bin/pint...

Just need to configure once into the pint.json

@danie-ramdhani
Copy link

This pr reminds me when laravel introduce return type when we create a new controller.
Explained: https://www.youtube.com/watch?v=daaT9HvotPU&t=5s&ab_channel=LaravelDaily

and same idea with:

Why not all classes in php is not final by default?

If this pr get merged:

What is this? laravel flavored next.js? when devs need use server or use client by default?

@ash-jc-allen
Copy link
Contributor Author

@nunomaduro Ahh yeah, sorry! I hadn't even thought about those types of things. Based on that, I'm going to make the assumption this PR will likely get closed (so it can potentially all be done at once in the future, if added).

But if you ever decide to add strict types to all the Laravel codebases, I'd love to be a part of the process and help out! So if you need an extra pair of hands, I'd be happy to help 🙂

@ash-jc-allen
Copy link
Contributor Author

I don't think that should be a concern for Laravel. Those who know what this is for will look to add to their files or edit the stubs of their projects.

And also we can add a declare(strict_types=1); into all files with a single command vendor/bin/pint...

Just need to configure once into the pint.json

Yeah that's something you can do. But, that requires the developer to know about strict type-checking for them to do that. A goal of this PR is to educate developers that didn't already know what it was 🙂

@DanielCoulbourne
Copy link
Contributor

I think the cost of adding extra lines to stubs is huge. The value may be there, but the argument seems a little vague in this case.

I don't think "educating developers about the existence of a feature" is a good enough reason to add extra code to a stub.

@ash-jc-allen
Copy link
Contributor Author

I think the cost of adding extra lines to stubs is huge. The value may be there, but the argument seems a little vague in this case.

I don't think "educating developers about the existence of a feature" is a good enough reason to add extra code to a stub.

Yeah, I don't think I'd realised how much extra work there'd be (like updating other codebases, code examples, etc).

That's totally cool though! I'd have kicked myself if I hadn't tried making the PR 🙂

@taylorotwell
Copy link
Member

Too close to L11 release to consider this.

@ash-jc-allen ash-jc-allen deleted the strict-type-stubs branch March 4, 2024 15:26
@caendesilva
Copy link
Contributor

I love using strict types, but I don't think it's a good fit to bundle with Laravel considering that Laravel uses magic casts and a strictness layer would be confusing. A better way to educate developers is by promoting the feature in blog posts and similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants