- 🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.
As a bug this will need a test case
Did not review or test.
- First commit to issue fork.
- First commit to issue fork.
- 🇪🇸Spain vidorado Logroño (La Rioja)
Fixed a couple of things and added a unit test covering the case :)
- 🇬🇧United Kingdom oily Greater London
Removed 'Needs tests' tag as a unit test is in place.
- 🇪🇸Spain vidorado Logroño (La Rioja)
vidorado → changed the visibility of the branch 11.x to hidden.
- 🇺🇸United States smustgrave
Feedback in MR seems legit.
Wonder if we have to worry about backwards compatibility and add a trigger_error
- 🇪🇸Spain vidorado Logroño (La Rioja)
@smustgrave, could you explain better what do you mean by backwards compatibility and in which case should we trigger an error?
- 🇬🇧United Kingdom oily Greater London
RE: #28 @vidorado Your changes to the code comments definitely makes their meaning clearer. RE: #27, the question could be raised in the Slack #core-development channel.
- 🇬🇧United Kingdom oily Greater London
The comments seem clearer now in line with recommendations.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.