Now that Fully packaged Drupal distributions now deployed on drupal.org is done, one of the things were discussed adding was a form on d.o that profile maintainers to use to verify a proposed drupal-org.make file, or to generate a proper one based on a list of projects. There are nice tools for this in drush make now, and they're documented, but not all profile maintainers are necessarily going to install drush make locally (unfortunately).

I'm not 100% sure if this UI should live in drupalorg.module or in project_package. Seems like it really belongs as drupalorg code, since it's specific to drush make, drupal distro packages, drupal-org.make, etc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hunmonk’s picture

Status: Active » Needs review

i agree that this belongs in the drupal.org customizations -- there's really no clean, secure way to make this generally useful outside of the drupal.org architecture. some of the interface would be reusable, but other things (like the conversion and verification code itself) would need to be hacked. living is drupal.org customizations is perfect, as it's publicly available for hackage if any body really wants to leverage it.

my initial stab at how it should work:

  1. a link on the profile project pages, "Manage packaging .make file", in the same set of links that contains "Add new release"
  2. clicking that takes you to a form which remembers what profile you're working with
  3. a set of radios at the top, with the choices "Convert an existing .make file to drupal-org.make format", and "Verify an existing .make file is in drupal-org.make format"
  4. a text area below to paste the .make file
  5. selecting a radio choice would pop up some simple instructions for that choice via js
  6. user selects option, pastes code, presses a 'Process' button at the bottom of the form
  7. submit function grabs the textarea input and pipes it to either 'drush convert makefile' or 'drush verify makefile', depending on the radio selection.
  8. if the convert/verify fails, we use drupal_set_message($output, 'error');, where $output is the output of the drush call.
  9. if the convert/verify is successful, we drupal_set_message(t('Your conversion|verification was successful'));, and if conversion was done, put the output of the drush call back into the textarea as the default value.

thoughts?

dww’s picture

Status: Needs review » Active

Sounds exactly like we discussed -- looks right to me.

Note to others looking at this: we fixed drush make so that it can do everything via STD(IN|OUT) -- so, the httpd process wouldn't need to write anything to disk at all when invoking drush make.

Back to active -- someone actually needs to write the code now. ;)

hunmonk’s picture

Assigned: Unassigned » hunmonk
Status: Active » Needs review
FileSize
8.67 KB

ok, here's the first draft -- it actually works quite well :)

A) i didn't think much about the names -- just used the first ones that seemed sensible. suggestions welcome.
B) my biggest concern is this section of code:

      $line = escapeshellcmd($line);
      // escapseshellcmd() clobbers the brackets, and we need them.
      // TODO: can we really know this is secure?
      $line = str_replace(array('\[', '\]'), array('[', ']'), $line);

i get parsing errors if i don't convert the brackets back. i'm really not sure if this is insecure or not -- feedback welcome.

hunmonk’s picture

Title: Add a UI on installation profile release nodes to verify/convert/generate a drupal-org.make file » Add verify/convert UI and release node verification for profile .make files
FileSize
14.93 KB

this version of the module is pretty well fleshed out -- it does both the convert/verify UI, and the release node verification, and it's installed and working as advertised on http://d6.project.drupal.org.

since it requires local shell access to the server to make adjustments necessary to test the release node verification (a few custom hacks were needed to make the module viable to deploy on the test site), it's best for folks to just test out the verify/convert UI.

this can be done by visiting any installation profile project page and clicking the 'Convert/verify release .make files' link in the release links section, and using the displayed form.

hunmonk’s picture

quick note about #3B: looks like in shell scripting, brackets are only used as a shortcut for tests, ie [ -d /var/local ]. doing echo "[ -d /var/local ]" in both sh and bash just outputs the quoted text, so i'm feeling pretty sure that how i handle the escaping of the multi-line input is ok.

hunmonk’s picture

FileSize
16.52 KB

attached is the result of several UI workover sessions w/ Bojhan. this now needs a sec. team review and it'll be ready to hit drupal.org

hunmonk’s picture

FileSize
15.96 KB

fix usage of wget, use escapeshellarg() where appropriate instead of escapeshellcmd() -- this also lets me get rid of the weird bracket hack i was using.

pwolanin’s picture

Taking a look - on the live server is drush going to be installed just in your account? Seems like it should be in /usr/local/bin maybe?

+define('PROJECT_VERIFY_PACKAGE_DRUSH_BIN', '/Users/hunmonk/bin/drush');
pwolanin’s picture

is this regex right? I thought it was more restrictive:

+// official release.
+define('PROJECT_VERIFY_PACKAGE_RELEASE_TAG_REGEX', '/^DRUPAL-(\d+)--(\d+)-(\d+)(-[A-Z0-9]+)?$/');
apaderno’s picture

I think the last part of an official release string should be just ALPHA, or BETA, or RC, or UNSTABLE, or an empty string followed by a number of maximum 2 digits.

pwolanin’s picture

On the staging site, bueditor attaches to the text area for e.g. verifying a make file - is that desired?

I'd also make the textfield have more rows by default.

dww’s picture

Status: Needs review » Needs work

Thanks for the reviews, pwolanin!

Re: #8: Yeah, that's unfortunate. We should install this in a better location on the real site.

Re: #9, #10: Right, the regexp from the CVS repo is this:

'@^DRUPAL-[567]--(\d+)-(\d+)(-(ALPHA|BETA|RC|UNSTABLE)[0-9]+)?$@'

Re: #11: That's just a bueditor configuration problem. We should reconfigure bueditor accordingly. More rows sounds reasonable.

Setting CNW based on the regexp and the extra rows. Haven't had a chance to actually review the code or test yet -- just got back from 2 weeks offline and scrambling to try to fix update manager before the 7.0-alpha1 freeze/release... ;)

hunmonk’s picture

Status: Needs work » Needs review
FileSize
15.79 KB

re: #8: forget about that -- it's just the setting for my local deployment, and it will be adjusted for the final deployment. drush already lives in an agreed upon place on the drupal webserver. attached patch just leaves those setting blank for now.

re: #9 & #10: that regex only needs to distinguish whether it's a branch tag or a release tag. it doesn't make sense to make it any more specific, because then it breaks if we add any new extra strings. i'm leaving that as is.

attached fixes the size of the textarea, setting #rows to 20 -- seems like a reasonable size on my screen.

chx’s picture

Status: Needs review » Reviewed & tested by the community

This is a security review (a proper review would wonder whether 'foo' . FOO .'bar' works for t() or not but it's after all, a constant string, so should). As a security review, it's short. There is no output. Good. There is a watcdhog. With a % placeholder. Go for it.

chx’s picture

Status: Reviewed & tested by the community » Needs work

dww points out the problem that this command actually runs code on drupal.org -- sorry was looking for common vectors. I am not proficient in this but wouldnt a popen be more secure than echo 'ing something escapeshellarg'd and then pipeing?

Damien Tournoud’s picture

This doesn't look *that* great from a code perspective (using proc_open() would make a lot more sense). But it does seem secure.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Then we are OK...

dww’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Usability
FileSize
148.87 KB
8.2 KB
16.44 KB

I just gave this a closer look. While there's a lot to like, I think this still needs some work before we can deploy it. I've rerolled some of the problems I found, but I don't have time to fix everything I found when trying to use it...

A) [Fixed] The two code comments at the top of the file, the @file and the warning about site-specific stuff, should be merged. The indentation was off on the @file comment, too.

B) [Fixed] This is weird:

<a href=\"!doc_link\">http://drupal.org/node/642116</a>

If we have define('DOCUMENTATION_LINK', 'http://drupal.org/node/642116');, we should use !doc_link as the URL text, too. ;) It's silly to have a constant for the URL that's different from what it links to.

C) [Fixed] Fixed the help text regarding when a release is invalid. Before:

The %makefile file for project %project_title failed verification for CVS tag %cvs_tag.\n\n<a href=\"!doc_link\">http://drupal.org/node/642116</a> -- to learn more about correcting these errors.\n\n!output\n\nOnce these errors are fixed, re-tag your project and submit the release node again.");

After:

"The %makefile file for project %project_title failed verification for CVS tag %cvs_tag.\n\n<a href=\"!doc_link\">!doc_link</a> -- to learn more about correcting these errors.\n\n!output\n\nOnce these errors are fixed, commit the changes to your %makefile, move the release tag for your project, and submit the release node again.");

D) It's not clear to me what's up with all these \n and then nl2br() peppered throughout. That's not how core usually handles multi-line messages. Ditto, none of this is translatable. IMHO, it'd be better if these were t()'ed, and wrapped in <p>, even if that means putting them in a function instead of a global define. It'd probably be cleaner in the code that way, too, since all the token values could be explicitly passed into this function.

E) [Fixed] project_verify_package_menu() is generating these menu callbacks for *any* node, so long as the current user can create release nodes. :( If you use %project_node, the menu items only exist for project nodes. See project_release_menu() for an example. The menu loader is in project.module itself.

F) [Fixed] Fixed PHPDoc to use doc standards and cleaned up some of the code logic in project_verify_package_verify_release_node().

G) There's a fair bit of code here, 95% of which doesn't need to be loaded on every d.o page load. This module should start life with a project_verify_package.inc file with most of the code and we conditionally load that when needed (e.g. if we're altering a release node, or the menu callbacks, which already have nice support for conditionally loading a file).

H) I agree that proc_open() would be a lot cleaner (and more efficient) for project_verify_package_run_drush_via_pipe() than concocting an "echo $stuff | something" command to exec. Plus, this 2>&1 is just mixing stderr and stdout, which is exactly what we don't want to be doing here. ;) We want the payload (stdout) separate from the warnings and other cruft. See below. Let's fix this function. proc_open() will make it much easier to read/maintain, and will also give us stdout and stderr in separate file streams, so we can handle them separately. Yay.

I) [Fixed] Simplified a bit of the code in project_verify_package_get_remote_file() and fixed/expanded the PHPDoc.

J) [Fixed] Cleaned up the PHPDoc for project_verify_package_format_exec_output() and added something explaining that it removes blank lines for you.

K) Actually trying to use the convert UI, it's pretty confusing. :( See attached screenshot. Imagine I'm a novice d.o distribution maintainer (the target audience). Exactly what part of that am I supposed to put in my drupal-org.make file? ;) There's 0 visual indication, just a wall of text. The most obvious thing would be the text area, select all, and paste. But, that's the input I started with, not the converted file. :( I think this really needs more work before we deploy it. If the primary motivation for this module is to make packaged distros more usable, we need to consider the UI/UX here... The main thing is we need the converted payload in an *obvious* UI element, either its own text area or at least a separate fieldset or something.

I didn't actually test out the release node validation stuff yet, only the verify + convert UI (I'm testing locally, not on d6.p.d.o)...

Attached is the updated patch with the fixes to the items here I prefixed with "[Fixed]" along with an interdiff against path #13 for comparison.

hunmonk’s picture

Status: Needs work » Needs review
FileSize
19.63 KB

attached should address all issues raised in #18.

dww’s picture

Status: Needs review » Needs work
FileSize
6.92 KB
20.03 KB

Fixed a few more things:

L) [Fixed] Added a comment in the .module config section pointing to the config defines in .pages.inc

M) [Fixed] Moved the form help text to a form element instead of using hook_help(). Not only does hook_help look bizarre in some themes (especially the d.o redesign), hook_help() text is really hard to alter or modify. I find doing help paragraphs in a #markup form element is way better for stuff like this.

N) [Fixed] Word on the street is that the potx extractor will almost certainly barf and complain with a menu definition that looks like this:

    'title' => 'Verify ' . PROJECT_VERIFY_PACKAGE_MAKE_FILE . ' files',

I don't think we're ever going to change this filename, so I just made the menu item titles simple strings, e.g.:

    'title' => 'Verify drupal-org.make files',

O) [Fixed] Cleaned up project_verify_package_run_drush_via_pipe()
-- Fixed typo in PHPDoc
-- Simplified some inline comments
-- Fixed $return vs. $return_value bug in the error case

P) I know I'm the one that suggested creating project_verify_package_cvs_error_messages() instead of those define constants. However, now that I see it more closely, we're only *ever* using these messages once. ;) Unless there's a compelling reason we think we're going to need to reuse them, I think they should just get folded into the function where they're used. I believe that'd make for simpler, more readable code. Thoughts?

Q) Playing with this some more -- is there actually a reason to have the verify and convert menu items live off specific projects at all? At first, I just assumed there was something about it where the project mattered. But, now that I look closely, the only reason we care what project we're on is to set the breadcrumbs. Huh? ;) Is this about a project maintainer retaining context for their project node while they're working on this stuff? Wouldn't this be simpler if these pages just always lived at a fixed location? Can you explain why you wrote it this way?

I don't think P or Q necessarily should block deployment, so feel free to ignore the CWN and just commit and deploy this patch. But, if it's quick easy to fix/justify either/both, please do.

Thanks!
-Derek

p.s. When you commit this to drupalorg, be sure to do all of the following:
- checkout a HEAD workspace of the drupalorg project
- apply the patch
- cvs add project_verify_package (adds the directory)
- cvs add project_verify_package/* (adds all the files)
- cvs commit -m "#647454 by hunmonk, dww: Added verify/convert UI and release node verification for profile .make files" project_verify_package (the new files now live in HEAD)
- cvs tag -b DRUPAL-6--1 project_verify_package (create a DRUPAL-6--1 branch for the new files, since that's what we actually deploy on d.o production these days)
- d.o-cvs-to-svn.php drupalorg DRUPAL-6--1 (import the latest from the end of DRUPAL-6--1 in CVS into d.o's SVN)
- cd production/sites/all/modules/drupalorg in your d.o SVN checkout
- svnmerge.py merge
...

hunmonk’s picture

Status: Needs work » Postponed (maintainer needs more info)

P) sure, i can move that -- it seems sensible. i originally created those defines to *try* and keep the module reusable by other folks, but i guess we're beyond that now, and should just code most simply to suit our needs

Q) how about /project-package-verify-makefile/[project_nid] and project-package-convert-makefile/[project_nid]? project_nid would be an optional argument, and if passed it could be used to load the project node to provide a better breadcrumb. we make use of project_nid in our links from the project pages to keep the current usability, while still providing a single home for these pages. thoughts?

dww’s picture

Status: Postponed (maintainer needs more info) » Needs work

Re (P): Sounds good. ;)

Re (Q): I don't think it's worth the effort if we're still doing the breadcrumb thing. If that's a good enough reason for these to live off each project node, fine with me. I'd only both changing it if we embrace the idea that this is just a service on d.o, sort of like part of the handbook or something, not anything that's specific to each project.

In other news, I was thinking when I woke up this morning about this issue and also the proposed module for parsing .info files to record project/release/module dependency data. As currently written, the .info patch is totally drupal-specific, there was no effort to separate the general from the specific. So, that's probably going to be punted into the drupalorg project, too. However, neither this nor that are necessarily drupal.org specific, they're just specific to sites using project* for managing drupal code. So, maybe we want a separate glue project, something like project_drupal, where these two (and other modules) live? See #102102-60: Parse project .info files: present module list and dependency information for more.

hunmonk’s picture

maybe we want a separate glue project, something like project_drupal,

i dunno. sounds like more maintenance overhead to me. i'd sooner keep everything in drupalorg, and realize it means 'a junk drawer of modules for the drupal infrastructure', rather than 'a junk drawer of modules for drupal.org'

hunmonk’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
18.49 KB

attached fixes P). i've also added '(check the CVS manual to learn how to move tags if necessary)' to the tag error message

skipping Q) since we seem to agree that it's fine as is.

i think this is ready to go.

hunmonk’s picture

Status: Needs review » Fixed

committed to CVS, SVN, and deployed on drupal.org -- hurrah!

apaderno’s picture

Wow. :-)

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

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

  • Commit e5fc2d4 on 6.x-3.x, 7.x-3.x-dev by hunmonk:
    #647454 by hunmonk, dww: Added verify/convert UI and release node...