Posted by Damien Tournoud on April 6, 2011 at 4:54pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | update.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | keichee |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
The "Manual updates required" in admin/modules/update is generated in the wrong format, and as a consequence has a spurious column:
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| manual-updates-required.png | 29.43 KB | Ignored: Check issue status. | None | None |
Comments
#1
It's not really pretty, but there is nothing better we can actually do :(
#2
I'm marking this as a duplicate of #1024990: "Manual updates required" table has an extra column, however you solve the problem in a different way. I'll like that other issue to this one.
#3
The patch in #1 is likely the simplest fix to this problem. I've tested it out and it works wonders. :) I marked the other issue, referred to in #2, as a duplicate of this one.
#4
Is the following summary correct?
We have a function that builds up an array that is either passed into theme('tableselect') or theme('table').
The problem is that theme('tableselect') and theme('table') take a different array as input.
This hack turns a theme('tableselect') array into theme('table') array.
If it is correct, I think we can better document it in the code. Setting back to 'needs review' as I'd like some extra input.
#5
this is a much simpler way that works..
#6
Patch looks good, but this isn't critical, and the patch doesn't look like it needs porting to me, ought to apply straight.
#7
Tested this manually, fixes the problem fine. I think we could still use a comment here though - the code is much simpler but we should explain why we're unsetting attributes.
#8
thats per entry, so core entry attributes have no attributes set, others will have.
thats a simple issue that no one will reuse and free some space.
#9
Dries' hypothesis in #4 is correct.
#10
#5 really looks like a hack.
#11
@Damien, without inline docs, both #1 and #5 look like hacks, except #5 is a lot more concise. So if #1 is the correct fix here and #5 is a nasty workaround, the docs should make that obvious. Marking CNW since neither are adding those.
#12
Just needs a comment like
+ // The elements of the projects array are each generally themed as a+ // tableselect giving checkboxes to chose which updates to install.
+ // To omit these checkboxes for projects where 'Manual updates required',
+ // we instead theme this section as a table. Unfortunately the variables
+ // for theme_tableselect and theme_table are not compatible, so we must
+ // first massage the data into an appropriate format.
#13
look, i just followed the existing code to handle core
existing code is
unset($entry['#weight']);but after i traced, weight is not even existing, i think weight is a typo and it must be an attributes.
try to print_r the $entry variable outside that if condition code that handle manual updates and see what i mean.
print_r($entry);my code is legit as per complying to the existing code how to handle manual updates.
#14
The patch in #1 with a comment similar to #12 should suffice.
#15
Marked #1200834: Ugly border on manual updates table on available updates report as a duplicate.
#16
I've updated the comment patch in #1 to address Dries' concern in #4.
#5 is a hack in the sense that it unnecessarily removes the attributes (i.e., 'class' => 'update-recommended') which may be used by the site's CSS.
#17
Marking this as RTBC since it is the same code as in #1 (which was marked RTBC in #3) and hopefully addresses Dries concerns with the comments.
#18
The new comment covers this well I think.
Committed to 8.x, moving back to 7.x for webchick to look at.
#19
w00t! Great to see this fixed!
Committed and pushed to 7.x.
#20
Automatically closed -- issue fixed for 2 weeks with no activity.