Closed (fixed)
Project:
Drupal core
Version:
7.x-dev
Component:
update system
Priority:
Normal
Category:
Feature request
Assigned:
Issue tags:
Reporter:
Created:
20 Aug 2009 at 14:08 UTC
Updated:
3 Jan 2014 at 00:29 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
jhodgdonYour new function header needs a small doc rewrite -- see http://drupal.org/node/1354
Comment #2
dwwI know it's confusing, but "update.module" is for the part of core (added in 6.x) that checks for available updates to your modules and themes. You're talking about update.php, which is the "update system" component...
p.s. Yes, the core components were temporarily scrambled for a little while due to #554978: Tabledrag breaks component UI for projects with tons of components -- now fixed on d.o.
Comment #3
gordon commentedI have added more documentation around what the function does and returns.
Comment #4
dww@gordon: The "I" in API stands for "interface", and adding something to the interface is a "change". ;) jhodgdon has been trying to tag API changing issues prior to code freeze so folks can focus on those while there's still time...
Comment #5
moshe weitzman commentedI agree with Gordon. There is no utility to grouping API additions with API change. The only promise we make is that we try not to break APIs after freeze, nor add new features. But an API addition is still plausible. Best to focus on patches that would break APIs
Comment #6
moshe weitzman commentedFound a few minor issues. This is a pretty useful patch.
Good idea to reset here. Start your comment with an uppercase.
An array *keyed* ...
following information => following keys
the => then
typo: requirements
This review is powered by Dreditor.
Comment #7
dwwMy point is that things like http://webchick.net/node/69 point to http://drupal.org/project/issues/search/drupal?issue_tags=API+change in an effort to draw attention to things people are trying to get in before code freeze. That is all.
Anyway, whatever... onto the patch. The PHPDoc was further messed up in ways that Moshe didn't mention (more typos, formatting, clumsy prose, etc). I fixed all that. I also formatted the PHPDoc so the list of keys for each subarray will render as an HTML list on api.d.o.
While I was at it, I decided that from a re-usable API standpoint, it'd be nice if the 'pending' subarray that's returned by update_get_update_list() was keyed by the update number itself, instead of just having that info embedded (along with the function descriptions) in the values of the array.
That said, yes, this is very helpful and a nice move towards making update.php scriptable and API-driven, instead of having everything tied into the UI. If this wasn't my own patch, I'd RTBC.
Comment #8
dwwOh, I missed the point of the reset and Moshe's first comment. dreditor seems to have failed in some way. ;) Anyway, yeah, resetting the statics inside update_do_one() is a nice touch. Fixed that comment, too.
Comment #9
gordon commentedYes the comment this looks great. I think it is ready to go.
Comment #10
dwwwebchick balked at the (basically unrelated) hunk for clearing the statics in update_do_one(). Fine, removed. That can happen in another issue.
Comment #11
webchickReview of #8:
Is this function only for modules, or all updates? If the former, could we include "module" in the name someplace? If the latter, why call this $module? It seems like Adrian's patch will make at least profiles updatable. Maybe $project would be more future-proof?
(Summary from IRC: can't use Project because multiple modules might be part of the same project. $name would be better. However, there are many parts of the API that use $module, so updating all of these should be a follow-up patch.)
As far as I can tell this has nothing to do with this API change. Let's move it to a separate issue for discussion.
I realize you are just re-shuffling code around, but this string appears to not be translatable. Do we need it to be? Might be worth making a follow-up to fix this.
Otherwise this looks good.
I'm on crack. Are you, too?
Comment #12
webchick#10 is the same as #8 with the questionable hunk removed. Committed to HEAD. Thanks!
Please update this issue with links to follow-ups.
Comment #13
moshe weitzman commentedPlease review the new issue for the drupal_static_reset() aspect of this patch which was pulled before commit - #557582: Reset statics after each update.
Comment #14
JacobSingh commentedWould it be possible to add an optional ($module) argument to the function?
Plugin Manager would want to get updates for an individual module. Scanning the whole tree is not the end of the world, but would be nice to be a little more efficient.