Closed (fixed)
Project:
Drush
Component:
PM (dl, en, up ...)
Priority:
Major
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
27 Nov 2010 at 22:32 UTC
Updated:
21 Dec 2010 at 01:00 UTC
Jump to comment: Most recent file
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | drush-dl-select-wout-bootstrap-5.patch | 32.68 KB | greg.1.anderson |
| #28 | drush-dl-select-wout-bootstrap-4.patch | 29.12 KB | greg.1.anderson |
| #17 | drush-dl-select-wout-bootstrap-3.patch | 24.73 KB | greg.1.anderson |
| #14 | drush-dl-select-wout-bootstrap-2.patch | 16.62 KB | greg.1.anderson |
| #13 | drush-983222.patch | 7.06 KB | jonhattan |
Comments
Comment #1
pcambraComment #2
pcambraNew version of the patch fixing naming of the functions and doxygen
Comment #3
greg.1.anderson commentedCool. 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.
Comment #4
moshe weitzman commentedI like Greg's enhancements in #3
Comment #5
jonhattanI 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.Comment #6
luchochs commentedAs 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).
Comment #7
jonhattanComment #8
greg.1.anderson commentedHere's a patch that implements the suggestions in #3.
Comment #9
greg.1.anderson commentedNote that this is also a handy workaround to the bug in project, #877988: Release history XML feed is missing <recommended_major> for some projects with a single release.
Comment #10
jonhattanHere 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?
Comment #11
greg.1.anderson commentedI 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.
Comment #12
greg.1.anderson commentedHere 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.
Comment #13
jonhattanGreat. 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)
Comment #14
greg.1.anderson commented@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.
Comment #15
greg.1.anderson commentedI'm going to make a minor change to #14 to fix the Drupal module dependency issue; patch coming shortly.
Comment #16
luchochs commentedIs it time for a new helper function? I think so.
Comment #17
greg.1.anderson commentedThis patch fixes #14 and #16, and makes pm-releasenotes no longer require a bootstrapped Drupal environment.
Comment #18
greg.1.anderson commentedI 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.
Comment #19
luchochs commentedSome tests tomorrow, but looks really great!
Comment #20
moshe weitzman commenteddrush_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.
Comment #21
greg.1.anderson commentedYes, 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.
Comment #22
moshe weitzman commentedOther 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)
Comment #23
luchochs commentedIt 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.
Comment #24
pcambraI'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.
Comment #25
jonhattanEven 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.Comment #26
pcambraOk, 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.
Comment #27
greg.1.anderson commented*_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.
Comment #28
greg.1.anderson commentedPatch eaten; here it is.
Comment #29
greg.1.anderson commentedThere 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.
Comment #30
greg.1.anderson commentedFixed 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.
Comment #31
jonhattanYep. I'm writting the commit message. It doesn't fit a tweet :p
Comment #32
jonhattanI'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 :/
Comment #33
jonhattanCommitted: http://drupal.org/cvs?commit=460196
Comment #34
greg.1.anderson commentedAwesome! 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.
Comment #35
greg.1.anderson commentedCommitted a fix for the warnings in drush_choice. http://drupal.org/cvs?commit=460254