Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In preparation to expose Drupal Components outside of Drupal we added a composer.json file to every component and a "replace" section to core/composer.json.
Right now drupal/core-render is missing from the drupal/core replace section.
Proposed resolution
- Fix the drupal/core replace section to have the drupal/core-render component declared.
- Add any external drupal/core-render requirements to drupal/core/composer.json and have composer update the other drupal/core/composer.* files.
- Add to Drupal\Tests\ComposerIntegrationTest::getPaths().
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | core_render_is_missing-2752119-12.patch | 509 bytes | mayurjadhav |
| #7 | 2752119_2.patch | 509 bytes | jalpesh |
| #4 | 2752119.patch | 486 bytes | jalpesh |
Comments
Comment #2
Eric_A CreditAttribution: Eric_A commentedComment #3
Eric_A CreditAttribution: Eric_A commentedComment #4
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commentedI think the issue is about to add drupal/core-render in composer.json file. I have attach a patch for same.
Comment #5
Eric_A CreditAttribution: Eric_A commentedYes, it needs to be added to the replace section in the composer.json file. These items are now in alphabetical order. Best to keep it that way. Could you adjust your fix? Thanks!
Comment #6
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commentedSure will do and update the patch. Thanks for quick response.
Comment #7
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commentedNew patch with alphabetical order. Thanks.
Comment #8
Eric_A CreditAttribution: Eric_A commentedGreat!
Comment #9
Eric_A CreditAttribution: Eric_A commentedComment #10
Eric_A CreditAttribution: Eric_A commentedComment #12
mayurjadhav CreditAttribution: mayurjadhav at Blisstering Solutions commentedMay be this will work in 8.1.x.
Comment #13
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commented@mayurjadhav,
It was already RTBC, I don't understand why do you need to submit a new patch. It was already assign to me and i was already working on it. we have already same discussion on other issue. Let me know if you don't understand what i am trying to tell you.
Comment #14
mayurjadhav CreditAttribution: mayurjadhav at Blisstering Solutions commented@jalpesh,
Issue was in Needs work state because your patch was failed, I submitted clean patch without fail. I don't think some one needs 6 days to work on such small fix.
Comment #15
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commented@mayurjadhav,
Did you see the old comment of this issue. It already tested by someone and mark it as RTBC in comment #8, I think you didn't even go through all comment before submitting a patch. All you need is Credit don't care about anything.
Comment #16
Eric_A CreditAttribution: Eric_A commentedSo i clicked the test report link in #7 ( https://www.drupal.org/pift-ci-job/343887) to see what the two fails were about. The report showed two unrelated test fails.
I'm going to request a retest and I expect to see confirmation that the patch from #7 still applies and still passes tests. The patch uploaded by @mayurjadhav differs by a line number offset of one, which should not make a difference.
So I plan to RTBC #7 once more when the re-test has come back green, but I would be very happy if mayurjadhav did so before I'm back.
The core committers will see to it that credit is given where credit is due. Also: when we succeed in showing collaboration, and perhaps some friendliness, calmness and patience, it is usually very much appreciated in this community.
Comment #17
Mile23+1 @Eric_A
For a non-code patches like this one, it's often likely that a testbot failure is unrelated, unless the patch no longer applies.
In that situation, it's best to re-run the test by clicking on 'add test' and re-running. You can also re-set the issue status to 'needs review' or RTBC because if the test fails again, it will be set back to 'needs work.'
Reviewing #7. Tests pass. The patch applies. The replaced package name matches the package name for drupal/core-render. It's in alphabetical order.
Setting to RTBC.
Comment #18
mayurjadhav CreditAttribution: mayurjadhav at Blisstering Solutions commented@Eric_A agreed. +1 for RTBC.
Comment #19
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commentedAgree with @Eric_A and @Mile23.
Comment #20
Eric_A CreditAttribution: Eric_A commentedExcellent. Time to get @jalpesh's work in, then.
Comment #22
Mile23Unrelated fail.
Restarting test in #7.
Comment #23
jalpesh CreditAttribution: jalpesh as a volunteer and at Cybage Software Pvt Ltd. commentedIt's look like #7 pass the test. Thanks.
Comment #25
Eric_A CreditAttribution: Eric_A commentedYes, it did and it does. (I re-queued the test after #7 failed again due to unrelated random failures.)
Comment #26
alexpottCommitted and pushed f51a120c3f575e678d1a95cd2d7fcac7f7e79a40 to 8.2.x and e2e3145 to 8.1.x. Thanks!
Comment #27
alexpott