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

[11.x] Fix configure() return type #49648

Closed
wants to merge 2 commits into from
Closed

[11.x] Fix configure() return type #49648

wants to merge 2 commits into from

Conversation

TomasVotruba
Copy link

@TomasVotruba TomasVotruba commented Jan 11, 2024

Hi everyone,

I've upgrade my personal and Rector website to Laravel 11 to try out new configuration API:

All is good!
One glitch is that the IDE autocomplete didn't work for me. I though there is some IDE cache, but in the end it was the return type issue.

This should fix it 👍

Using native return type seems a good idea, as we're using PHP 8.2 as min, and the incorrect return native type declaration would show this bug even in the PR adding this feature.

Let me know what you think and what should I improve 🙏

@TomasVotruba TomasVotruba changed the base branch from 10.x to master January 11, 2024 11:25
@@ -221,9 +222,8 @@ public function __construct($basePath = null)
* Begin configuring a new Laravel application instance.
*
* @param string|null $baseDirectory
* @return \Illuminate\Foundation\ApplicationBuilder
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class does not exist, Configuration was missing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type-ezgif com-cut

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return type into doc

@driesvints driesvints changed the title [Laravel 11] Fix configure() return type [11.x] Fix configure() return type Jan 11, 2024
@driesvints
Copy link
Member

Thanks @TomasVotruba. Instead I just fixed the docblock itself: af5ba26

@driesvints driesvints closed this Jan 11, 2024
@TomasVotruba TomasVotruba deleted the tv-typed-builder branch January 11, 2024 13:45
@TomasVotruba
Copy link
Author

Thanks 👍

Just curious, why not change the type declaration?

@driesvints
Copy link
Member

We don’t really add native return types to internal code.

@TomasVotruba
Copy link
Author

Why is that?

@driesvints
Copy link
Member

Just because that’s how we’ve always been doing it

@TomasVotruba
Copy link
Author

I see. Would you be open to use native return types?
I'd be happy to help as it's my daily bread if there is interest.

@driesvints
Copy link
Member

Not right now sorry. We’re going to leave things be 👍

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.

3 participants