Fix the documentation for #empty_value on Drupal\Core\Render\Element\Select

Please ignore the 10.1.x, I pushed there by mistake. The same fix is in the 3361695-fix-the-documentation branch.

Additionally, should the Form API or render system detect this problem and raise a waring? Some kind of render/form API array structure validation

Creating list of files to check by comparing branch to 10.1.x
/var/www/html/core/lib/Drupal/Core/Render/Element/Select.php:44:38 - Unknown word (overriden)

Should be overridden instead.

Thanks for the review, typo fixed and branch rebased so it's mergeable again.

Seems there are out of scope indent changes happening.

Ouch! I'm trying a new IDE and I've discovered it is not configured for Drupal coding standards. Sorry for the noise.

Text appears to match the issue described in the issue summary and no issues in the tests.

LGTM.

I think adding an assert (as proposed in #5) for this condition would be a great DX improvement since this would be a real DrupalWTF to deal with.

#12, I think it is interesting to automatically check that the issue is not happening (aka #options value overriden by #empty_value if #empty_option is present in #options), but I'm not sure how to do it.

Using assert will only work when assertions are enabled. This can be the case for automatic testing but it is not on production environments. Given the dynamic nature of forms, it is possible that the case where #options is overriden only happens on certain situations. This would not be detected by testing. And having a fatal error can be too drastic, I guess.

I would check the condition and raise a warning. This way Drupal is able to continue running, even though there's probably a problem. Any devs checking the logs or debugging a weird behavior on a form will see error.

But this discussion can ne longer, and the initial issue is about documentation. I think we should focus this issue on the documentation and open a new one for the warning where we can discuss how to do it.

Maybe open a follow up for #12?

Follow up added: 🐛 Add an exception/warning/assert/other when a #options value is overriden because #empty_options Needs work .

I am doing triage on the core RTBC queue → .

The issue summary is short and clear. And looking at the MR it is fixing the problem stated there. I read the comments and see that a suggestion was moved to another issue. That looks like a good choice here.

On reading the MR I though the sentence could be improved and made a suggestion on the MR. Setting back to NR for that.

Then, I realized I should have read the code. It appears this is working as designed, therefor I am not convinced that the doc change should include 'Warning'. Right now I am thinking the sentence should begin with 'Note that' instead of 'Warning:'

For the update mentioned in 16

#16, it may work as designed, but the behavior doesn't seem right, hence the warning. See https://www.drupal.org/project/documentation/issues/1936636#comment-1505... → for an example.

However, I think "Note"wold work. If it is preferred I won't complain, but I would like to have a decision soon so we can fix the MR with the agreed value.

Searching core found examples of using both Warning: and Note: so seems like a coin toss. Going to mark it though as the sentence has been updated.

+1 that "note that" would read better here. It's definitely more in line with our pre-existing comments.

Changed "Warning: " by "Note that" (with an intermediate commit where I wrongly used "Note: ")

Seemed to cause CC failure.

PHPStan step has been executed successfully.

Thanks for the quick fix. Going to go ahead and mark it as I doubt this will cause test failures.

I'm triaging RTBC issues → . I read the IS and the comments since my last comment. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

I've probably come to lurk on this issue more than 3 times by now and I've always gotten stuck at trying to remember what does it mean that the #empty_option overrides a value from the #options. I remember this made sense after stepping through this with a debugger but maybe we need to improve the docs so that that wouldn't be required? 😅

@laurii I guess you mean more details in the sentence are required on the proposed sentence.

This is the current sentence:

Note that if #empty_value is the same as a key in #options then that value in #options is overridden with the value of #empty_option

Would it be clear with this?

Note that if #empty_value is the same as a key in #options then the value of #empty_option is used for that key in the #options array.

Also, I guess the status should be "Needs work" instead of "Needs review", right?

Does sound like NW is the correct status.