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

[12.x] use promoted properties #53834

Closed
wants to merge 2 commits into from

Conversation

browner12
Copy link
Contributor

this new feature helps reduce a lot of boilerplate code.

resubmission of #53807

I had originally started the original PR against master, but then was concerned if there were changes prior to the v12 release, we might run into some annoying merge conflicts. as long as we're not concerned about that too much, then this master targeted PR should be good.

this new feature helps reduce a lot of boilerplate code.
@browner12 browner12 marked this pull request as ready for review December 10, 2024 18:04
@mnabialek
Copy link
Contributor

My only concern here is that comments for properties that contains some more info are not there anymore.

@Rike-cz
Copy link

Rike-cz commented Dec 10, 2024

Sorry, but this is really not good pattern.

  1. part of properties get shrink into constructor and lost full comment,
  2. part of properties stay on top as usual.

Together it brings worse readability because both types of declaration have different "rhythm".

@rodrigopedra
Copy link
Contributor

Can't we keep the properties comments in the constructor docblock?

Copy link
Contributor

@JurianArie JurianArie left a comment

Choose a reason for hiding this comment

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

Type hinting the callables as \Closure might be a breaking change for some people. Before this change callable instances were also accepted

@donnysim you are right, I only looked at the properties but did not check the constructor.

@donnysim
Copy link
Contributor

@JurianArie i don't see any type hints being changed here tho 🤔 they all were and still are Closure type.

@taylorotwell
Copy link
Member

I do think I'll just leave this off for now since we lose information from docblocks.

@browner12 browner12 deleted the AB-modern-constructors-2 branch March 17, 2025 16:47
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.

7 participants