As a further expansion of the update.inc into making it a much more useful api to reduce the overlap between update systems, I have created a new function update_get_update_list() which will build the list of pending updates for the updater to use.

This also reduces some of the code in the update.php file as well.

Comments

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +API change

Your new function header needs a small doc rewrite -- see http://drupal.org/node/1354

dww’s picture

Component: update.module » update system

I 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.

gordon’s picture

I have added more documentation around what the function does and returns.

dww’s picture

Issue tags: +API change

@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...

moshe weitzman’s picture

Issue tags: -API change

I 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

moshe weitzman’s picture

Found a few minor issues. This is a pretty useful patch.

+++ includes/update.inc
@@ -419,3 +421,60 @@ function update_finished($success, $results, $operations) {
+ *   An array listed by module is returned which contains all information that

Good idea to reset here. Start your comment with an uppercase.

+++ includes/update.inc
@@ -419,3 +421,60 @@ function update_finished($success, $results, $operations) {
+ * @return
+ *   An array listed by module is returned which contains all information that
+ *   is needed to inform the user of the updates, and any updates that are not

An array *keyed* ...

+++ includes/update.inc
@@ -419,3 +421,60 @@ function update_finished($success, $results, $operations) {
+ *   For each module the following information is returned in the array.

following information => following keys

+++ includes/update.inc
@@ -419,3 +421,60 @@ function update_finished($success, $results, $operations) {
+ *       the do not process any updates for this module as there are other

the => then

+++ includes/update.inc
@@ -419,3 +421,60 @@ function update_finished($success, $results, $operations) {
+ *       requirments that need to be sorted.

typo: requirements

This review is powered by Dreditor.

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new6.97 KB

My 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.

dww’s picture

StatusFileSize
new6.97 KB

Oh, 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.

gordon’s picture

Status: Needs review » Reviewed & tested by the community

Yes the comment this looks great. I think it is ready to go.

dww’s picture

StatusFileSize
new6.54 KB

webchick balked at the (basically unrelated) hunk for clearing the statics in update_do_one(). Fine, removed. That can happen in another issue.

webchick’s picture

Review of #8:

+++ update.php	21 Aug 2009 05:35:18 -0000
@@ -48,51 +48,29 @@ function update_script_selection_form() 
+  $updates = update_get_update_list();
+  foreach ($updates as $module => $update) {

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.)

+++ includes/update.inc	21 Aug 2009 05:35:18 -0000
@@ -332,6 +332,8 @@ function update_do_one($module, $number,
+    // Conserve memory by resetting all the static variables.
+    drupal_static_reset();

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.

+++ includes/update.inc	21 Aug 2009 05:35:18 -0000
@@ -417,3 +419,63 @@ function update_finished($success, $resu
+        $ret[$module]['warning'] = '<em>' . $module . '</em> module can not be updated. Its schema version is ' . $schema_version . '. Updates up to and including ' . $last_removed . ' have been removed in this release. In order to update <em>' . $module . '</em> module, you will first <a href="http://drupal.org/upgrade">need to upgrade</a> to the last version in which these updates were available.';

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?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

#10 is the same as #8 with the questionable hunk removed. Committed to HEAD. Thanks!

Please update this issue with links to follow-ups.

moshe weitzman’s picture

Please review the new issue for the drupal_static_reset() aspect of this patch which was pulled before commit - #557582: Reset statics after each update.

JacobSingh’s picture

Would 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.

Status: Fixed » Closed (fixed)
Issue tags: -API addition

Automatically closed -- issue fixed for 2 weeks with no activity.