This is the first step (I think) towards what we discussed in the Boston sprint.

To summarize the objective is to simplify the core pm code (which has some hideously long and complex functions!), make it easier to work on improved release/upgrade information management code that is not so tied to update module, and to allow other systems (dog being the prime candidate) to "own" all file management (including both the "package" and version control aspects). Hence the idea is to refactor pm and separate out the release information fetching and file management code into separate engines (Drush-speak for pluggable codebases).

The current patch contains 2 fairly minor changes to start:

1) Make drush_include_engine() return an appropriately named class, if there is one. Probably we can convert most/all of the current engines into interface based classes. It may make sense to replace this with some PSR-0 autoloader, but I figure we can do this later.

2) Move some basic remote metadata fetching functions for pm-download into a release_info engine type (not a class for now, since the interface is still in flux). Note that this is a separate purpose from the update_info engine type, which is responsible for determine a list of updates given a set of projects (not directly fetching any actual release information).

As far as I can tell, this shouldn't actually change anything - it just moves things around. Tests pass.

After that, some of the next steps would probably look something like this:

3) Start identifying and moving all code that examines or modify the site filesystem into a new engine that would encapsulate both the package_handler and version_control engine types. As a reminder, the idea is that this forms an abstraction layer that dog or similar systems could use where package_handler and version_control are overlapping/interdependent. I haven't worked out a name yet - one idea is to use the package_handler namespace, and move the old engines into package_fetcher or something. Either way, this is going to need some serious untangling to get things straight.

We can then work on some of the bigger picture items - for example:

* Helping dog implement the "high level" file mangling interface (whatever we call it), so that pm commands can be used transparently on dog sites and can share the same basic project/release information sources/logic.

* Writing an alternate update_info engine that uses the release_info interface to determine possible upgrades (avoiding the update.php dependency).

* Writing another release_info engine that uses an sqlite database (or some other system) to provide lower latency and more detailed project information.

* Probably a few other things that we can fill in as we go ;)

CommentFileSizeAuthor
pmtng1.patch21.21 KBowen barton

Comments

moshe weitzman’s picture

moshe weitzman commented

Looked at the patch and it all looks sane to me. Would be good to get feedback from jonhattan.

A couple random comments ...

  1. I'm not sure dog will ever become real.
  2. MarkS and I have been talking about moving drush_make into core. I'd like to borrow the file format and parsing from the current project, but otherwise do a new implementation of the processing of rows in the .make. Would affect this issue.
owen barton’s picture

owen barton commented

Yes - it would be great to get jonhattan's input! Even if dog never comes to pass, separating out the file management code is still good from a simplicity point of view, I think - and I am pretty sure we/someone could muster some kind of submodule-based functionality, which needs this in any case. I am not familiar with drush_make internals - would be interesting to figure out how it muxes into this though.

msonnabaum’s picture

msonnabaum commented

I've been meaning to suggest putting in a PSR-0 autoloader for 5. The one I'm using for drush deploy I put together with the intention of bringing into core:

http://drupalcode.org/project/drush_deploy.git/blob_plain/HEAD:/DrushAut...

It's basically the stock symfony autoloader with the apc caching built in rather than separated into a different class.

I'm also seeing a lot of make overlap, and I'm really starting to like the idea of pulling it's functionality into core. It would be a nice manifest that we could use with dl/update-code instead of guessing which version is best. The only big additions to our codebase would be pulling down files via cvs/svn/bzr, but the code that covers that in make is not that huge.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

I'd be interested in helping integrate drush make into core drush.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

I created #1310130: Put drush make in drush core to track progress.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs review » Reviewed & tested by the community

The introduction of release_info engine is definitely a great step to clean up pm commands. Its a nice strategic viewpoint. I did fail some tries to refactoring all of this due to its complexity.
Also keeping it decoupled of update_info is strategic to walk towards #1032626: Allow drush pm-update w/o update.module and help fixing #1305428: Don't check ALL modules for updates in pm-update when specific modules are specified and other several issues related to drush usage of update.module. So I'm all for this patch.

Dreditor is broken in my browser. So here is a craft review:

1. As drush_include_engine behavior changes (or is extended) it merits a documentation line

2. It seems you missed moving _drush_pm_get_releases_from_xml() to release_info engine. Perhaps it is to be merged with release_info_fetch(). Anyway we can do it later after this patch.

3. 

-class drush_pm_version_control_backup implements drush_pm_version_control {
+class drush_version_control_backup implements drush_pm_version_control {

The class inteface could be renamed drush_version_control.

4. 

+/**
+ * @file Drush release info engine for update.drupal.org
+ */
+
+function release_info_fetch(&$request) {

Note that we support other than u.d.o services. Namely features servers. I'm fine with naming the release_info engine as "drupalorg" but let's no forget it is about drupalorg-compatible services.

Other than that, it is RTBC for me. Great work!

ps. I'll write all considerations related to drush_make in #1310130: Put drush make in drush core to not pollute this one.

greg.1.anderson’s picture

greg.1.anderson commented

Also agree that this is good. My one dissension is that I would rather not see function / class / package handler names with "drupalorg" in it. The function / class / package handler name should indicate what the behavior of the code is; drupalorg is where the updates come from, which should be specified by an argument, field, or option. e.g. --package-handler=git and _drush_pm_parse_release would be improvements, for example. However, I'm happy to see this code go in; we can always argue semantics later.

owen barton’s picture

owen barton commented
Title: PMTNG » PMTNG part 1
Status: Reviewed & tested by the community » Fixed

Implemented the above fixes (engine is now called updatexml) and committed.

Closing - will open a new issue for part 2 (of N)!

moshe weitzman’s picture

moshe weitzman commented
Issue tags: +Needs change record

Needs change notification

Status: Fixed » Closed (fixed)

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

moshe weitzman’s picture

moshe weitzman commented
Issue tags: -Needs change record

Not quite sure what to say here so removing Tag