Let's challenge this part of in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
// All props should also support "object" this allows deferring
// rendering in Twig to the render pipeline.
$type = $prop_schema['type'] ?? '';
$schema['properties'][$name]['type'] = array_unique([
...(array) $type,
'object',
]);
Remove this. Check if the tests are still OK.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13603
Comments
Comment #3
pdureau commentedNeeds review
Comment #4
pdureau commentedRemove "11.3.0 release priority" tag because it is not blocking #3352063: Allow schema references in Single Directory Component prop schemas anymore and can be merged after 11.3 if we need more time.
Comment #5
shubham_pareek_19 commentedreviewing it.
Comment #6
pdureau commentedThanks @shubham_pareek_19
I will also ping the original author of this snippet to have its opinion and maybe some information about the context of this addition.
Comment #7
pdureau commentedSome information from Mateu, the original author (not a proper quote):
Today, we have slots and we don't pass renderables in props. That may be the reason why this mechanism looks ok to be removed. Anyway, we need to be careful.
Comment #8
smustgrave commentedThink it needs a rebase based on the composer changes in the diff
Comment #9
pdureau commentedWeird. Because I don't think I have changed the composer files
Ah ok, it is because I have created the branch from 11.3.x but I am targeting 11.x. OK, I rebase
Comment #10
pdureau commentedComment #11
pdureau commentedComment #12
geek-merlinI have objections, likely remediable. No time now but promise will schedule time tommorrow.
Comment #13
shubham_pareek_19 commented@geek-merlin i have tested this MR this works fine for me. Waiting for your input.
thankyou.
Comment #14
geek-merlinOK, i needed to read the source in order to really grok it. If i get it right, it's removes an obsoleted internal type hack, so my +1 for that.
Review: Code looks straightformward: Remove that hack, adjust tests.
I would RTBC that.
Strange JS test failure though - https://git.drupalcode.org/issue/drupal-3554720/-/jobs/7076303
NW for that. Or to scrutinize and confirm unrelated (can be RTBCed then imho).
Comment #15
shubham_pareek_19 commentedComment #17
needs-review-queue-bot commentedThe 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.
Comment #18
pdureau commentedIt was RTBC, the rebase went fine, so back to "Needs review"
Comment #19
smustgrave commentedRestoring status!
Comment #20
alexpottI think this looks fine and conceptually correct but I think we should cover ourselves with a CR. If someone does want an object here then they'd have to add it to their schema right? Setting to needs work to add the CR.
Comment #21
pdureau commentedRight. Drupal must work with the schema authored by the component owner. It is not taboo to do some alteration Drupal side, we are already doing it with
meta:enumfor example, but let's try to avoid this when we can.On it.
Comment #22
pdureau commentedCR: https://www.drupal.org/node/3589343
Comment #26
catchThis looks good, thanks for the investigation into how this originally came to be. Forgot to credit @eoipso in the commit message but have credited on the contribution record.
Committed/pushed to main and 11.x, thanks! Updated the CR versions to 11.5 and published it with a couple of minor copy edits.