Currently, project_release_find_latest_releases() uses the file timestamp from files attached to release nodes to order releases that have extra (e.g. to know that rc1 is actually newer than beta4, etc). However, we shouldn't assume that release nodes have files attached to them at all. We should either use LEFT JOIN instead of INNER JOIN for the file stuff, or better yet, consider something else to ORDER BY when dealing with releases with version_extra defined. Perhaps the simplest would be to just use the nid. It's a bit of a hack, and potentially wrong, but there's no way to do anything like the logic from version_compare() inside the SQL query.

Our options:

A) Punt and use nid.

B) Query every release from the given branch without an ORDER BY in the SQL, slurp all the releases into RAM, and then sort in PHP via version_compare() or something like it.

C) LEFT JOIN on {project_release_file} and {files} so we use the file timestamp if available.

I'm not 100% sure the best approach here. (B) would be the most bullet-proof in some ways, but it also forces a very specific set of conventions on what version_extra can contain. (A) avoids that problem, but if people get crazy and add release nodes out-of-order, they'd get potentially weird behavior. (C) mostly works okay on sites that are using files, but fails on sites that don't attach files at all -- plus, (C) actually has the same problems as (A) in terms of releases created out of order...

I'm tempted to just go with (A) and document it. Perhaps we could have a knob for this that determines if we use version_compare() or minor/patch/nid to sort releases. Or, maybe the crazy solution is to use an internal view to build this query, and let sites do whatever they want if they need to change it: #642094: Move the code to build the query that drives project_release_find_latest_releases() into a shared helper function -- that case, I guess we'd provide some kind of version_compare() views sort handler, I'll try to check with Earl to see how possible/easy that is.

Comments

dww’s picture

Assigned: Unassigned » dww

No matter what way we go, I'm going to be solving this in the next day or two -- so assigning to myself.

hunmonk’s picture

#642094: Move the code to build the query that drives project_release_find_latest_releases() into a shared helper function certainly seems like the most flexible solution, if it's not too much to wrangle. if it's possible, i'd vote that route and make the default implementation A) from above -- we could even punt on the alternate sort handlers for now, and add those later if/when we need them.

dww’s picture

Title: project_release_find_latest_releases() should not assume files are attached to release nodes » Provide a better mechanism for sorting releases with version_extra defined: introduce version_extra_weight
Category: bug » task

After a good discussion with Earl about this and #642094: Move the code to build the query that drives project_release_find_latest_releases() into a shared helper function we came up with an even better option here. Although it's a bit of work, I think it's the best trade-off of complexity, accuracy, and flexibility:

D) We should introduce a new {project_release_nodes}.version_extra_weight field. This will hold an integer code for sorting different possible prefixes of version_extra. There could be an admin tabledrag UI to order the prefixes accordingly, and add others. Any version_extra value that doesn't start with one of the known prefixes would use a low weight so that those come "first" (and are just sorted alphabetically). This would effectively let us implement version_compare() in SQL. For example, we might have:

[unknown] = 0
unstable* = 10
alpha* = 20
beta* = 30
rc* = 40
NULL = 100 (i.e releases without extra are always last -- the final official release).

The actual values don't matter, just the relative order. So, in the query to sort releases on a given branch, if we wanted the newest/best releases first, we'd have the following ORDER BY clauses:
// We always want the dev snapshots to show up last.
$orderby[] = 'r.rebuild';
$orderby[] = 'r.version_minor DESC';
$orderby[] = 'r.version_patch DESC';
$orderby[] = 'r.version_extra_weight DESC;
$orderby[] = 'r.version_extra DESC;

Make sense? To pull this off, we'd need:
- UI for defining the weights (could punt for now and hard-code a mapping in settings.php or something)
- Schema change to add the new field
- Batching schema update to populate the new field based on existing values of version_extra and the mapping
- Code in project_release when we're creating/editing arelease node to compute the version_extra_weight based on the current value of version_extra
- Exposing this field to views

Should be relatively quick, especially if we punt the admin UI for now, and it should completely solve the problem in a flexible, accurate way. It's also going to make #642114: Compute and maintain the update status for every release node a lot easier.

hunmonk’s picture

this proposal looks good to me. definitely punt on the UI stuff now.

i suppose the batch update would have some array at the top that people could configure for their version extras, and then we put a big fat warning about setting that in an upgrade text?

dww’s picture

Status: Active » Needs review
StatusFileSize
new6.96 KB

Handles everything except the admin UI and exposing to views. Tested locally, works like a charm, both with the default mapping and a custom mapping in settings.php.

dww’s picture

BTW, here's a mysql command from my local test DB. Notice that the nids are "out of order" relative to the proper sorting of the versions (in particular, the 1.3-unstable1 release):

mysql> SELECT nid, version FROM project_release_nodes WHERE pid = 9 AND version_api_tid = 5 AND version_major = 1 ORDER BY rebuild, version_minor DESC, version_patch DESC, version_extra_weight DESC, version_extra DESC;
+-----+----------------+
| nid | version        |
+-----+----------------+
|  25 | 1.3-final-rc-1 | 
|  21 | 1.3-rc2        | 
|  20 | 1.3-rc1        | 
|  24 | 1.3-unstable1  | 
|  19 | 1.2            | 
|  18 | 1.2-rc2        | 
|  17 | 1.2-rc1        | 
|  16 | 1.2-beta1      | 
|  15 | 1.1            | 
|  14 | 1.1-rc1        | 
|  11 | 1.0            | 
|  10 | 1.0-beta1      | 
+-----+----------------+
12 rows in set (0.00 sec)
dww’s picture

StatusFileSize
new8.92 KB

Now with a fix to project_release_query_releases_by_branch() to use the new fields in the ORDER BY.

Tested locally and it's working great...

mysql> SELECT nid, version FROM project_release_nodes WHERE pid = 9 AND version_api_tid = 5 AND version_major = 1 ORDER BY rebuild, version_minor DESC, version_patch DESC, version_extra_weight DESC, version_extra DESC;
+-----+----------------+
| nid | version        |
+-----+----------------+
|  27 | 1.3-final-rc-2 | 
|  25 | 1.3-final-rc-1 | 
|  26 | 1.3-rc4        | 
|  28 | 1.3-rc3        | 
|  21 | 1.3-rc2        | 
|  20 | 1.3-rc1        | 
|  24 | 1.3-unstable1  | 
|  19 | 1.2            | 
|  18 | 1.2-rc2        | 
|  17 | 1.2-rc1        | 
|  16 | 1.2-beta1      | 
|  15 | 1.1            | 
|  14 | 1.1-rc1        | 
|  11 | 1.0            | 
|  10 | 1.0-beta1      | 
+-----+----------------+
15 rows in set (0.01 sec)
mysql> SELECT recommended_release, latest_release FROM project_release_supported_versions WHERE nid = 9 AND tid = 5 AND major = 1;+---------------------+----------------+
| recommended_release | latest_release |
+---------------------+----------------+
|                  19 |             27 | 
+---------------------+----------------+
1 row in set (0.00 sec)
hunmonk’s picture

patch looks very good. have not tested.

dww’s picture

Status: Needs review » Active

At hunmonk's request, I added the following comment:

      // If the $prefix exists inside version_extra, we have a match. We use
      // === 0 to tell the difference between the prefix being at position 0
      // (start of the string) vs. strpos() returning FALSE (not found).
      if (strpos($node->project_release['version_extra'], $prefix) === 0) {

Otherwise, committed this to HEAD. Yee haw!

Back to active for the UI and views support, but at least #642112: Add a latest_security_release column to {project_release_supported_versions} and #642114: Compute and maintain the update status for every release node are no longer blocked. ;)

dww’s picture

Assigned: dww » Unassigned
Status: Active » Fixed

Let's fix the views support with a bunch of other columns over at #644258: Finish exposing {project_release_nodes} data to views
The admin UI now lives at #644262: Add UI for controlling the version_extra_weight prefix mapping

So, this is now fixed.

Status: Fixed » Closed (fixed)
Issue tags: -packaged install profiles

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