Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#17 | drupal-2008982-16.patch | 2.12 KB | c4rl |
#16 | twig-2008982-16.patch | 985 bytes | hussainweb |
#13 | twig-2008982-13.patch | 4.64 KB | adamcowboy |
#13 | interdiff.txt | 1.12 KB | adamcowboy |
#11 | twig-2008982-10.patch | 674 bytes | adamcowboy |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2
xxAlHixx CreditAttribution: xxAlHixx commentedWDG (Ukraine,Kharkov) want to implement this on CodeSprintUA.
Comment #3
xxAlHixx CreditAttribution: xxAlHixx commentedComment #4
podarokcode styling errors
https://drupal.org/coding-standards#indenting
needs work
Comment #5
xxAlHixx CreditAttribution: xxAlHixx commentedI've fixed code styling errors
Comment #6
podarok#5 looks good
Comment #7
catchWe can assign the variables directly to $table instead of assigning to $variables then later to $table.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedComment #9
adamcowboy CreditAttribution: adamcowboy commentedDibs!
Comment #10
eromero1 CreditAttribution: eromero1 commentedDibs!!
Comment #11
adamcowboy CreditAttribution: adamcowboy commentedDone!
Comment #13
adamcowboy CreditAttribution: adamcowboy commentedTake 2!
Comment #14
jenlamptonGreat job adam :)
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedhuh?
Comment #16
hussainwebFixing the patch.
I also considered removing drupal_render as it seemed trivial. But didn't considering the guidelines in #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(). This is just a plain fixed patch.
Comment #17
c4rl CreditAttribution: c4rl commentedYeah, not sure I understand that last one. Let's try this one.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedYeh, the guidelines say:
The patch where I changed things, #2006974: Replace theme() with drupal_render() in views_ui.module was the first patch to go up and it still hasn't been reviewed while every straight conversion patch has been committed within a week or so of it being cleared of any code style issues. Waiting for manual testing is slooow.
My vote would be for the patch in #16 to just get the theme() removal out of the way, with a followup issue to make it more like #17 after the commit to get rid of the "flat" #markup attribute.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commented#16: twig-2008982-16.patch queued for re-testing.
Comment #20
heddn#16 needs to be re-rolled. Don't re-roll #17. See #18.
Comment #21
heddnIgnore #20. My bad.
Comment #22
heddn#16 looks good to me.
Comment #23
catchThis looks like it doesn't need #markup at all, could just leave the table element unrendered.
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedSure, well the patch in #17 still applies against HEAD so we just need some manual testing of the final markup.
Comment #25
hussainwebI am doing a round of manual testing. Any particular steps I should follow?
Comment #26
hussainwebI followed instructions on Managing configuration in Drupal 8 and copied all .yml files from a previous D8 instance to a fresh HEAD installation. I clicked on 'View Differences' for one of the changes config files and this is what I got.
I then installed the patch at #17. I cleared the cache and viewed the difference for the same config file. I verified the changes were in effect by temporarily altering some strings and seeing them in output. This is the HTML after the change:
I ran above through diff and there was no difference. Visually too, there is no difference whatsoever.
Comment #27
catchCommitted/pushed #17 to 8.x, thanks!