Download & Extend
Drush » Issues

Drush prints out HTML into terminal

Posted by kotnik on November 28, 2011 at 3:38pm
Project:Drush
Version:All-versions-5.x-dev
Component:Make
Category:bug report
Priority:minor
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

Noticed this while playing with #1268416: Destination directory .../drupal-7.9 already exists..

Drush up gives me output like this:

Checking available update data ...
Checked available update data for <em class="placeholder">Administration menu</em>.

It should look saner:

Checking available update data ...
Checked available update data for Administration menu.

Patch attached below.

Login or register to post comments

Comments

#1

Posted by kotnik on November 28, 2011 at 3:40pm

Patch attached.

AttachmentSize
clean_output-1354778-1.patch 916 bytes

#2

Posted by moshe weitzman on November 28, 2011 at 11:19pm
Status:needs review» needs work

Please provide more detail. Drupal7 tries not to print html in its errors. What code is doing thle placeholder?

#3

Posted by kotnik on November 29, 2011 at 9:25am
Status:needs work» needs review

Unfortunately it still does leak HTML if it's batching. Code that outputs HTML is here, %placeholder returns HTML as you can see here.

#4

Posted by greg.1.anderson on November 29, 2011 at 9:37pm

Maybe we need to add strip_tags to $value in includes/batch.inc?

class DrushBatchContext extends ArrayObject {
  function offsetSet($name, $value) {
    if ($name == 'message') {
      drush_log($value, 'ok');
    }
    elseif ($name == 'error_message') {
      drush_set_error('DRUSH_BATCH_ERROR', $value);
    }
    parent::offsetSet($name, $value);
  }
}

#5

Posted by greg.1.anderson on November 29, 2011 at 9:38pm

(Only add strip_tags to the parameter $value as passed to drush_log and drush_set_error; do not perturb parent:offsetSet.)

#6

Posted by kotnik on November 30, 2011 at 9:40am

Greg, you are right, this is better place to do it. Patch attached.

AttachmentSize
clean_output-1354778-6.patch 901 bytes

#7

Posted by bforchhammer on December 5, 2011 at 4:16pm
Status:needs review» reviewed & tested by the community

Patch in #6 works :)

(Tested with drush up command).

#8

Posted by moshe weitzman on December 6, 2011 at 10:15pm
Version:All-versions-5.x-dev» All-versions-4.x-dev
Status:reviewed & tested by the community» needs work

committed to master. seems safe for 4.x but does not apply cleanly.

#9

Posted by greg.1.anderson on December 6, 2011 at 10:22pm
Status:needs work» patch (to be ported)

Set status.

#10

Posted by kotnik on December 12, 2011 at 10:11am

Batching approach changed in #687724: 'multithreaded' updatedb for better memory consumption and there is no need for this patch for 4.x, since there are no issues there with HTML output.

#11

Posted by greg.1.anderson on December 12, 2011 at 4:46pm
Version:All-versions-4.x-dev» All-versions-5.x-dev
Status:patch (to be ported)» fixed

Sounds right to me.

#12

Posted by Rob Loach on December 13, 2011 at 8:29pm
Status:fixed» needs review

What if instead calling strip_tags() all the time, we used dt()?

AttachmentSize
dt.patch 1.35 KB

#13

Posted by moshe weitzman on December 13, 2011 at 9:16pm
Status:needs review» fixed

Not going to translate arbitrary strings. Drupal doesn't do that either.

#14

Posted by Rob Loach on December 13, 2011 at 9:27pm
Component:Miscellaneous» Make
Status:fixed» needs review

The reason I bring that up is because this looks kind of silly.

AttachmentSize
strip_tagsALLTHETHINGS.patch 9.05 KB

#15

Posted by Rob Loach on December 14, 2011 at 7:35pm

Ah, I understand. This is the same issue, but occurring in different places. I'm running into HTML being outputted when running through Drush Make's download factory. I think we have two options here...

1) drushmakehtml.patch
Stick a strip_tags() in the dt() function so that the <em> tags are removed.
2) checkplainInsteadOfEm.patch
Switch the dt() parameters to use @ instead of % so it uses check_plain() on the parameters instead of wrapping it in an em tag.

I think I prefer the #2 checkplainInsteadOfEm.patch approach more. Let's use @ instead of % for those dt() calls.

AttachmentSize
drushmakehtml.patch 691 bytes
checkplainInsteadOfEm.patch 8.83 KB

#16

Posted by greg.1.anderson on December 14, 2011 at 7:42pm
Status:needs review» reviewed & tested by the community

I agree that #2 is preferable.

#17

Posted by greg.1.anderson on December 17, 2011 at 6:19am
Status:reviewed & tested by the community» fixed

Committed.

#18

Posted by System Message on December 31, 2011 at 6:20am
Status:fixed» closed (fixed)

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

#19

Posted by vinmassaro on May 9, 2012 at 8:59pm
Status:closed (fixed)» active

I still see this with Drush 5.1:

$ drush urol "administrator" --uid=2
Added the <em class="placeholder">administrator</em> role to uid <em class="placeholder">2</em>                                                                           [success]

#20

Posted by moshe weitzman on May 10, 2012 at 7:35pm
Status:active» closed (fixed)

Please use latest dev snapshot before reporting bugs. That might be fixed already. If still broken, feel free to reopen.

#21

Posted by vinmassaro on May 11, 2012 at 2:45pm

My mistake, I did not see that this thread was related to Drush Make. This is an issue when using core commands. I'll open another issue.