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.
Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
To Test:
1. Add file field to article
2. Create an article upload a file
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal-file-2009014-36.patch | 746 bytes | StephaneQ |
#31 | After Edit Field.jpg | 52.8 KB | adamcowboy |
#31 | After Field Icon.jpg | 19.19 KB | adamcowboy |
#31 | Before Edit Field.jpg | 53.33 KB | adamcowboy |
#31 | Before Field Icon.jpg | 19.93 KB | adamcowboy |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2
Samvel CreditAttribution: Samvel commentedComment #3
Samvel CreditAttribution: Samvel commentedattached
Comment #4
podarok#3 works for me
rtbc
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commentedIn theme_file_formatter_table() I don't think we need to drupal_render() all the table cells here If we structure $table correctly.
IIRC, in _theme_table_cell() it looks like if we use the 'data' syntax for individual table cells theme_table() will call drupal_render() for us.
instead of:
could we try:
Comment #6
Samvel CreditAttribution: Samvel commentedAre you sure? Because seems i try this, and it not workink, because drupal_render(which will render $table) will not render anythinh in #rows.
Comment #7
thedavidmeister CreditAttribution: thedavidmeister commentedgive me a minute, I'll do a test to double check.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedWorks for me, you have to set 'data' for the actual cell in the row. That said, I could see the table when I printed $table rendered but not when it was returned? not sure if I was doing something wrong there.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedComment #10
c4rl CreditAttribution: c4rl commentedComment #12
c4rl CreditAttribution: c4rl commentedComment #13
thedavidmeister CreditAttribution: thedavidmeister commentedmissed a drupal_render() here?
The changes here mean we'll have to manually test the output, with particular attention to the trailing whitespace that was in #markup.
Was there another variable in this scope called $file_icon? if not, can we call $icon_build $file_icon?
needs manual testing.
also needs manual testing.
also needs manual testing.
Comment #14
c4rl CreditAttribution: c4rl commentedYeah, I shouldn't write patches at 1:00 at night. :) I'll have another look at this tomorrow.
Comment #15
c4rl CreditAttribution: c4rl commentedSome improvements and fixes now that I am awake.
Comment #17
c4rl CreditAttribution: c4rl commentedComment #18
jenlamptonneeded a reroll
Comment #20
hussainwebI see one of the problems here:
I also have doubts about this part:
But I am letting this part be as it seems it was working fine in #15 (at least tests passed).
Comment #21
hussainwebComment #23
hussainwebI am repeating the following change for theme_image_widget() which should fix those exceptions.
Interdiff is attached for clarity.
Comment #24
ErikV CreditAttribution: ErikV commentedI think this needs a reroll
Comment #25
benjifisher@ErikV:
Why do you say it needs a reroll? The patch applies cleanly locally. I will ask the testbot for its opinion.
Comment #26
benjifisher#23: drupal-file-2009014-23.patch queued for re-testing.
Comment #27
star-szr#23: drupal-file-2009014-23.patch queued for re-testing.
Comment #29
star-szrTagging for reroll.
Comment #30
benjifisherIt looks as though #2036087: Add public identifier to all render methods in all views plugins in core (commit 31efff2) conflicts. I feel like living dangerously, so I will manually edit the patch, adding "public" in the one context line that matches "function render". My patch applies cleanly, but what will the testbot say?
Comment #31
adamcowboy CreditAttribution: adamcowboy commentedI applied the patch and tested it. It worked well.
I also manually tested.
Attached are screen shots of Field Icon and File Upload Field.
Comment #32
adamcowboy CreditAttribution: adamcowboy commentedComment #33
adamcowboy CreditAttribution: adamcowboy commentedRTBC.
Comment #34
alexpottCommitted bc65847 and pushed to 8.x. Thanks!
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commentedThere is a theme() call in file_save_upload() still
Comment #36
StephaneQComment #37
sbudker1 CreditAttribution: sbudker1 commentedThe patch worked! There were no apparent issues when I uploaded a file in an article.
Comment #38
xjm#36: drupal-file-2009014-36.patch queued for re-testing.
Comment #39
webchickCommitted and pushed to 8.x. Thanks!
Comment #40
alexpottHad to revert this commit because the commit contained changes from #2052995: Add a route to the frontpage see f4aead7
Committed 9614f73 and pushed to 8.x.
Comment #41
alexpottCommitted 8b7bc0c and pushed to 8.x. Thanks!
Comment #42
webchickD'oh. Sorry about that!
Comment #43.0
(not verified) CreditAttribution: commentedInstructions how to test the patch