Hi

When you do a drush dl project and that project hasn't a recommended release, this message is displayed:
There is no *recommended* release for (etc)

I think it would be better to suggest the active releases to the user and that he/she chooses between them

I'm attaching a patch for pm.drush.inc that adds the suggestion feature.

Comments

pcambra’s picture

StatusFileSize
new2.95 KB
pcambra’s picture

StatusFileSize
new2.94 KB

New version of the patch fixing naming of the functions and doxygen

greg.1.anderson’s picture

Status: Active » Needs work

Cool. A couple of suggestions, though.

First, it might be handy to use this feature even if there is a recommended release. Maybe you want to install the latest unstable dev, but you're feeling too lazy to type the whole version string (been there). It would be helpful if this feature allowed you to select from the available releases if you specify an option (e.g. drush dl views --select).

Second, some projects have a lot of versions, and it is unlikely that anyone will want to select a really old release. The list of available versions should ideally be filtered. Every supported and recommended release should be included, and the most recent dev version should be included. For other versions, that are not dev or supported or recommended, maybe show only the first three unless --all is specified.

moshe weitzman’s picture

I like Greg's enhancements in #3

jonhattan’s picture

I also agree with #3.

Some more things to improve:
1. We should show a message to the user before the choice. For example a reduced version of the big message. "no recommended release found. select one of those available:"...
2. At present the big message will be relegated to when running drush outside a drupal root. Not very needed there. Perhaps we can completely get rid of the 'ask the maintainer to review...'.
3. There's an edge case we don't handle: unpublished projects. Ej: drush dl z. It shows the big message (not applicable). It's needed to check <project_status>unpublished</project_status>. So $options will never be empty.

luchochs’s picture

As far as I'm concerned, the suggestions to improve the proposal are very good, but I also would be happy with the original proposal per se (and maybe #5-1 instead of the current message).

jonhattan’s picture

Priority: Normal » Major
greg.1.anderson’s picture

Status: Needs work » Needs review
StatusFileSize
new3.9 KB

Here's a patch that implements the suggestions in #3.

greg.1.anderson’s picture

jonhattan’s picture

StatusFileSize
new4.88 KB

Here is #8 plus a early test for unpublished projects. With this test, $options will never be empty, as projects are always valid ones.

@greg I don't understand "Defaults to downloading the recommended release." for --select option. There is no default option but 0 => Cancelled, isn't it?

greg.1.anderson’s picture

Status: Needs review » Needs work

I don't have time to fix this right now, but #8 (and ergo #10) is no good, because it does not work unless you have a bootstrapped environment. Since dl works without a bootstrapped environment, I think that maybe we can build the options choices from the info available rather than using pm-releases. As it is right now, --select will go ahead an dl the recommended release without prompting if you're not bootstrapped, and that's just not good.

greg.1.anderson’s picture

Status: Needs work » Needs review
StatusFileSize
new7.07 KB

Here is a patch that prints out the available releases to download without requiring a bootstrap. pm_releases is not used; an equivalent list is built from the releases xml that pm-download pulls down from d.o.

As an added bonus, the flag --dev can now be specified if you would like to download the most recent development release. --select --dev will show you all of the development releases and let you pick one.

jonhattan’s picture

StatusFileSize
new7.06 KB

Great. I was thinking of making pm-releases non dependant on drupal. We can both commands share this functionality in a follow up issue.

I've done some changes:
1. check $recommended_major and $supported_majors have a value.
2. supress $project argument to _drush_pm_download_releases_choice() - not used.
3. removed this: ($releases is never empty at this point)

-    if (empty($releases)) {
-      drush_log(dt('There is no *recommended* release for project !project on Drupal !drupal_version. Ask the maintainer to review http://drupal.org/node/197584 and create/recommend a release in order to be compatible with drush and the drupal.org security broadcast system. A recommended development snapshot release is sufficient. Alternatively, run pm-releases command and explicity pm-download any non-recommended release that might be available.', array('!drupal_version' => $request['drupal_version'], '!project' => $request['name'])), 'ok');
-      continue;
greg.1.anderson’s picture

Title: Suggest releases to download when there is no recommended release » Suggest releases to download when there is no recommended release; use pm-releases without bootstrapping Drupal
StatusFileSize
new16.62 KB

@jonhattan: sorry, I was working on this the same time you were, and obsoleted your patch.

Now pm-releases is not dependent on Drupal. It will still use a bootstrapped Drupal instance, if available, to report on the installed version.

This patch also makes pm-releasenotes very nearly not dependent on Drupal, but it's not quite there. The project info structure is compatible, so it's just down to some minor changes.

This patch also improves drush_choice to use drush_print_table so that you can align columns when requesting a choice. This makes the output of dl --select look a lot more like the output of pm-releases, which greatly enhances readability.

I probably won't have time to work on this again this weekend. I'll put a note here if I pick it up again.

greg.1.anderson’s picture

Status: Needs review » Needs work

I'm going to make a minor change to #14 to fix the Drupal module dependency issue; patch coming shortly.

luchochs’s picture

+++ commands/pm/pm.drush.inc	4 Dec 2010 19:20:52 -0000
@@ -1715,6 +1862,56 @@ function drush_pm_download_validate() {
+    // Don't rely on UPDATE_DEFAULT_URL since perhaps we are not fully
+    // bootstrapped.
+    $url = drush_get_option('source', 'http://updates.drupal.org/release-history') . '/' . $request['name'] . '/' . $request['drupal_version'];
+    drush_log('Downloading release history from ' . $url);
+    // Some hosts have allow_url_fopen disabled.
+    if (!$xml = @simplexml_load_file($url)) {
+      if (!drush_shell_exec("wget $url")) {
+        drush_shell_exec("curl -O $url");
+      }
+      $filename = explode('/', $url);
+      $filename = array_pop($filename);
+      $xml = simplexml_load_file($filename);
+      drush_op('unlink', $filename);
+    }
+    if (!$xml) {
+      // We are not getting here since drupal.org always serves an XML response.
+      drush_set_error('DRUSH_PM_DOWNLOAD_FAILED', dt('Could not download project status information from !url', array('!url' => $url)));
+      continue;
+    }
+    if ($error = $xml->xpath('/error')) {
+      drush_set_error('DRUSH_PM_COULD_NOT_LOAD_UPDATE_FILE', $error[0]);
+      continue;
+    }   

Is it time for a new helper function? I think so.

greg.1.anderson’s picture

Status: Needs work » Needs review
StatusFileSize
new24.73 KB

This patch fixes #14 and #16, and makes pm-releasenotes no longer require a bootstrapped Drupal environment.

greg.1.anderson’s picture

I should explain that #17 introduces a new constant DRUSH_BOOTSTRAP_MAX. This will cause the command to do a DRUSH_BOOTSTRAP_FULL if possible (all the way to LOGIN, actually), or it will stop at DRUSH_BOOTSTRAP_DRUSH if that is as far as it can go. The 'drupal dependencies' are only evaluated if the bootstrap was successful; it is up to the command to confirm that Drupal is bootstrapped before using the dependent modules.

luchochs’s picture

Some tests tomorrow, but looks really great!

moshe weitzman’s picture

drush_pm_releases_init() and drush_pm_releasenotes_init() need not exist because of DRUSH_BOOTSTRAP_MAX constant?

I have not reviewed the pm stuff here but I trust you guys to do it nice.

On a related note, I got burned in test-run recently when its 'drupal dependencies' silently increased its bootstrap level to FULL. I think we should error in this case.

greg.1.anderson’s picture

Yes, the *_init() methods that call drush_bootstrap_max can be replaced with the DRUSH_BOOTSTRAP_MAX constant.

I fixed a bug in the 'drupal dependencies' code, but I'll double-check the results. It should be an error to have 'drupal dependencies' unless you bootstrap a Drupal site or use DRUSH_BOOTSTRAP_MAX.

I'll post a new patch, but leaving this as 'needs review', as more tests are welcome.

moshe weitzman’s picture

Other commands use drush_bootstrap_max() and can probably use the new constant instead. pm-status comes to mind, along with help (probably hard to change this one)

luchochs’s picture

It seems appropriate to mention that I will not have time to test; too many hours without sleep, my brain needs rest. Surely someone else can help with this.

pcambra’s picture

I'm getting this error on drupal 6 using the patch in #17, however, the releases are displayed correctly.

date(): It is not safe to rely on the system's timezone settings. You[warning]
are *required* to use the date.timezone setting or the
date_default_timezone_set() function. In case you used any of those
methods and you are still getting this warning, you most likely
misspelled the timezone identifier. We selected 'Europe/Berlin' for
'CET/1.0/no DST' instead in
drush/commands/pm/pm.drush.inc on line 1048.

In D7 works fine.

jonhattan’s picture

Even greater. Some notes:

1. drush_html_to_text() can be placed in drush.inc and be a wrapper around drupal_html_to_text() same as dt() is for t().
2. _drush_pm_download_file() can be renamed to drush_dowload_file() in drush.inc. At least wget package-handler and DRUSH_BOOTSTRAP_DRUSH when it downloads table.inc can benefit.
3. _drush_pm_filter_releases() -> it is only used by _drush_pm_download_releases_choice()
4. I think we don't need anymore update_status for pm-download / pm-releases. We have everything in the new custom code. The check for installed projects is being worked in #977048: Unable to detect module version information. Code there will only need to check if we're bootstrapped.
5. I don't understand this constructions "$release_type" != "Bug fixes". Is this a cast to string? Per my tests in #13 it is not needed.

pcambra’s picture

Ok, the errors in #24 are because D7 defines a default timezone and when you use it for D6 or withour bootstrap there is not timezone defined.
I've changed date.timezone in my php.ini to set one by default as jonhattan suggested and the error went away.

greg.1.anderson’s picture

*_init() methods that call drush_bootstrap_max have been replaced with the DRUSH_BOOTSTRAP_MAX constant.

'drupal dependencies' is definitely not increasing the bootstrap. Your command will fail if Drupal is not bootstrapped sufficiently (unless it uses DRUSH_BOOTSTRAP_MAX, in which case it must test that Drupal is bootstrapped before using dependent modules).

drush_html_to_text() has been moved to drush.inc

drush_download_file now in drush.inc, but I did not change other code to use it. The code that downloads table.inc is more resilient than drush_download_file; unification would benefit all callers.

_drush_pm_filter_releases is now also called by pm-releases, which now shows only a short list of releases, just like drush dl --select, unless the --all flag is specified.

The algorithm for _drush_pm_filter_releases is now different (improved). Only the first release of any given type (Security update, recommended, development, etc.) is shown, and releases that are only bugfix / feature releases are not shown. The exception to this is, if there is an installed release, then all releases newer than the installed release are shown. I think this pretty much shows the releases that the user will usually be interested in, and leaves out the rest. The --all flag is always available for those rare times you want to see every single release. (For the vanity poster?)

I suspect that what you say is true -- that if we merged this feature with #977048: Unable to detect module version information, then we might be able to get rid of the update status version of the algorithm and just pull the installed release from our code. I don't know what effect this might have on pm-updatecode, though; I'll have to leave this work to you. Is there time before drush-4? I'll try to review 977048 soon.

You were correct about "$release_type"; it was a cast to string, equivalent to (string)$release_type, left over from earlier iterations of the code and no longer necessary. I fixed it up.

Regarding #26, I don't know if there is any action needed in drush. Just documentation? Would it help if we called date_default_timezone_set() to "GMT" if the timezone isn't set in php.ini? Separate issue, maybe? I'd like to see these large issues committed.

greg.1.anderson’s picture

StatusFileSize
new29.12 KB

Patch eaten; here it is.

greg.1.anderson’s picture

Status: Needs review » Needs work

There are some problems with #28.

drush dl will download to the cwd rather than sites/all/modules, even when a Drupal module is bootstrapped.

There is some potential that pm-releases might not print any releases at all if there is no recommended or supported or security or development release.

greg.1.anderson’s picture

Status: Needs work » Needs review
StatusFileSize
new32.68 KB

Fixed problems in #29.

Fixed some problems with the algorithm that determined the supported and recommended release.

Removed the validation checks in pm-download that required a bootstrapped site in order to use --notes, since that works now.

In pm-releasenotes, get the status message from the info from _drush_pm_get_releases rather than calculating it locally in pm-releasenotes.

jonhattan’s picture

Status: Needs review » Reviewed & tested by the community

Yep. I'm writting the commit message. It doesn't fit a tweet :p

jonhattan’s picture

I suspect that what you say is true -- that if we merged this feature with #977048: Unable to detect module version information, then we might be able to get rid of the update status version of the algorithm and just pull the installed release from our code. I don't know what effect this might have on pm-updatecode, though; I'll have to leave this work to you. Is there time before drush-4? I'll try to review 977048 soon.

I'll provide a new patch for 977048 in short. It seems clear to me we can completely get rid of the usage of update_info engine in all commands *but* updatecode.

I've finally commited #31 with some little fixes and docstrings.

The problem @pcambra had in #26 is a combination of php 5.3 and drupal 6,5 or no site. drupal7 sets the default timezone.

---
It seems we have a warning in Console_Table when there's only a row to print :/

$ drush pm-download media
There is no recommended release for project media.
Choose one of the available releases:
Undefined offset:  3 table.inc:578                                                                                 [warning]
Undefined offset:  4 table.inc:578                                                                                 [warning]
Undefined offset:  5 table.inc:578                                                                                 [warning]
Undefined offset:  6 table.inc:578                                                                                 [warning]
 [0]  :  Cancel                                      
 [1]  :  6.x-1.x-dev  -  2010-Jul-11  -  Development 
jonhattan’s picture

Status: Reviewed & tested by the community » Fixed
greg.1.anderson’s picture

Awesome! Thanks for the commit; glad to have this in. I agree that we can get rid of the update module for everything except updatecode; the drush versions of the release management functions are only missing info on the installed release, and they're much faster than relying on update module. Looking forward to the patch.

I'll fix the Console_Table thing in a separate issue; we just need to fill out the rows so they're all the same width when $options is an array.

greg.1.anderson’s picture

Committed a fix for the warnings in drush_choice. http://drupal.org/cvs?commit=460254

Status: Fixed » Closed (fixed)

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