#Novice

I am adding this patch to resolve above issue . Please help me validate the same.

nishavaghela → changed the visibility of the branch 3501102-cannot-modify-readonly to active.

nishavaghela → changed the visibility of the branch 3501102-cannot-modify-readonly to hidden.

Automatically closed - issue fixed for 2 weeks with no activity.

Automatically closed - issue fixed for 2 weeks with no activity.

@benjifisher Thank you for reviewing and for your kind words! I’m excited to contribute to Drupal core, and I appreciate your guidance. Looking forward to making more contributions!
@quietone Thank you merging.

quietone → closed merge request !10961

I forget how often but api.drupal.org isn't updated daily.

Committed c6bc6eb and pushed to 11.x. Cherry picked to 11.1.x

Thanks!

I've tried applying patch #99 - 3171835-98-horizontal-tab.patch and it did not solve the issue for me.

I have a required field in a fieldset that is on a horizontal tab:

@kul.pratap:

Thanks for making the changes I suggested!

I see that you have several contribution credits already, but none yet for Drupal core. Maybe this will be your first. Good luck, and I hope you keep contributing!

I reviewed the changes in the MR, and they do what I requested in the issue summary. These changes are all on comment lines (the class doc block) so there is no need for testing.

Once these changes are merged into the 11.x branch, we should see them in the API docs: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Plugi....

> $this->app = $this->prophesize(HttpKernelInterface::class);

Good point about tests. I can see 3 tests in core/tests/Drupal/Tests/Core/StackMiddleware that mock an http kernel, and they get stacked with other kernels.

It's highly unlikely someone would be doing step debugging here -- IIRC it all goes haywire with mocks -- but let's change it for consistency.

> Is there a grep command to confirm that all instances have been changed?

I came up with this when I posted the issue:

> `ag -G Middleware __construct`

After rebase unrelated Nightwatch test is also green now.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thanks all, fixed

ivnish → closed merge request !21

@anybody if the MR looks good then please merge and close the issue.

ivnish → changed the visibility of the branch 3.0.x to hidden.

I read the IS, comments and the MR. I didn't find any unanswered questions.

Is there a grep command to confirm that all instances have been changed?

And what about tests? For example. this changes the name in core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php but not in the associated test, core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php. That seems odd, at least to me. I think it would be better to change standardize them all. Is there a plan to do this in tests? I am not one to expand scope but it might make sense here.

PHPStan currently does not have a rule for checking less specific return types. Psalm does.
I'v just created a feature request for PHPStan project.
https://github.com/phpstan/phpstan/issues/12442

and how will we prevent future problems?

For now that may just have to be the Human Review process.

Core could create a PHPCS rule that the description must contain a format like “True if . False if .” when BOOL is used however that might be a bit more disruptive. I would suggest such should be a follow-up issue as fixing the broken docs is the bigger concern.

Core adopting higher level PHPStans would also help long term but is outside the scope or this issue.

What is the plan here so that we know that they are all fixed

Some missed would be better than none fixed.

I think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php.

@quietone true and false are covered there in value types section.

Automatically closed - issue fixed for 2 weeks with no activity.

I think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php. And I do understand that this change helps.

There are currently 1064 instances of bool in and @return statement. What is the plan here so that we know that they are all fixed and how will we prevent future problems?

Needs rebase and deprecation for 12.x

Automatically closed - issue fixed for 2 weeks with no activity.

Automatically closed - issue fixed for 2 weeks with no activity.

#6: you can choose :)

FYI: one rename for consistency is already happening in 📌 Improve or remove ComponentSourceInterface::getClientSideInfo() Active 🥳

🐛 StreamWrapperInterface realpath() and dirname() return docblocs are is inconsistent with documented use, actual Core implementation, and intention Needs work as related, it discusses some of the streamWrapper returns including LocalStream::getLocalPath()

Not reviewed the MR, however a general +1 to the concept to cleanup documented return types as this hurts contrib that run at PHPStan and will eventually mask errors in Core that PHPStan can detect.

Agree this isn't a Coding Standard issue.