Problem/Motivation

Would be great to simplify installation process of simplehtmldom library.

Proposed resolution

Implement Drush command that would download and install simplehtmldom library automatically.

CommentFileSizeAuthor
#15 simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-15.patch8.44 KBchernous_dn
#13 simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-13.patch10.08 KBchernous_dn
#9 simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-9.patch13.21 KBchernous_dn

Comments

nikolay shapovalov’s picture

nikolay shapovalov commented

+1

Anonymous’s picture

Anonymous (not verified) commented

Failing all else, this module should provide a graceful error message for those of us who didn't read the Readme before installing (especially through drush, like I did) to the effect of:

"This module requires the simplehtmldom library from http://sourceforge.net/projects/simplehtmldom/" rather than crash the site with the White Screen of Death.

Update
Thanks, konstantin.komelin. Sowee for not seeing that other tracked issue.

Konstantin Komelin’s picture

Konstantin Komelin commented

Thank you Borden. We already have an issue for #2, it's #2199489: Implement hook_requirements for simplehtmldom library.
Feel free to contribute with a patch.

Konstantin Komelin’s picture

Konstantin Komelin commented
Status: Active » Needs work
Issue tags: +#DCAmsterdam2014

Scheduled this task to DrupalCon Amsterdam.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -#DCAmsterdam2014 +Amsterdam2014

Let's use Amsterdam2014 tag, thanks!

Konstantin Komelin’s picture

Konstantin Komelin commented

Sure. Thank you Gábor!

Konstantin Komelin’s picture

Konstantin Komelin commented
Issue tags: -Amsterdam2014
Konstantin Komelin’s picture

Konstantin Komelin commented
Title: Implement Drush command for installing simplehtml library » Implement Drush command for installing simplehtmldom library
chernous_dn’s picture

chernous_dn commented
StatusFileSize
new simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-9.patch13.21 KB

I created drush command, to download library after enable module.

Konstantin Komelin’s picture

Konstantin Komelin commented
Status: Needs work » Needs review

Thanks @Chernous_dn for your patch. I will review it as soon as possible.

Konstantin Komelin’s picture

Konstantin Komelin commented
Assigned: Unassigned » Konstantin Komelin
Konstantin Komelin’s picture

Konstantin Komelin commented
Status: Needs review » Needs work

Hi @Chernous_dn,

I can't apply your patch because of its encoding or some other issues.
Please check patches before submitting:
git apply --check simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-9.patch
And make sure that it is in UTF-8.

Thanks,
Konstantin

chernous_dn’s picture

chernous_dn commented
StatusFileSize
new simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-13.patch10.08 KB

I made ​​a mistake when creating a patch and forgot to test. Вut now I have created a proper patch and tested it.

Konstantin Komelin’s picture

Konstantin Komelin commented

Hi @Chernous_dn,

Thank you for your efforts. The patch looks better now but not yet ideal.

1) First of all, I received the following warning:

warning: 2 lines add whitespace errors.

I guess that's because you didn't include new lines at the end of some files. Please also test your patch without --check parameter before submitting.

2) You have introduced new conditions in the hook_requirements() but we don't need them right now.
Please leave the same conditions untouched but add phase check to be able to install module even if the library is not installed:

if ($phase == 'runtime') {
   // the same conditions...
}

Let me know if you have strong arguments to change the conditions.

3) Please make sure that a punctuation sign follows any comment sentence, for example

// Decompress the zip archive

should be:

// Decompress the zip archive.

Comment standards https://www.drupal.org/node/1354

4) If it's possible let's add one more alias for the drush command - 'simplehtmldom'.

5) Please remove all files and folders except simple_html_dom.php file from sites/all/libraries/simplehtmldom through your drush command .
Some examples or documentation files may contain vulnerabilities and we should protect our users from being hacked.

Thanks,
Konstantin

chernous_dn’s picture

chernous_dn commented
Status: Needs work » Needs review
StatusFileSize
new simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-15.patch8.44 KB

This patch all of your comments and recommendations. This patch covers all questions in the previous comments.

alexander_danilenko’s picture

alexander_danilenko commented 
+++ b/drush/simplehtmldom.drush.inc
@@ -0,0 +1,133 @@
+    $path = 'sites/all/libraries';

+++ b/simplehtmldom.install
@@ -9,25 +9,65 @@
+        $requirements['simplehtmldom_plugin'] = array(
+          'title'       => t('simplehtmldom plugin'),
+          'value'       => $error_type,
+          'severity'    => REQUIREMENT_ERROR,
+          'description' => t('!error You need to download the !simplehtmldom, extract the archive and place the simplehtmldom directory in the %path directory on your server.',
+            array(
+              '!error'         => $error_message,
+              '!simplehtmldom' => l(t('simplehtmldom plugin'), $library['download url']),
+              '%path'          => 'sites/all/libraries'
+            )),
+        );

Path hardcoded? What if user will prefer to use multisiting?

+++ b/drush/simplehtmldom.drush.inc
@@ -0,0 +1,133 @@
+    $elements_to_remove = array(
+      'app',
+      'example',
+      'manual',
+      'testcase',
+      '.gitignore',
+      'change_log.txt'
+    );
+
+    foreach ($elements_to_remove as $elemt) {
+      drush_delete_dir($elemt);
+    }

Is there no other way to exclude all files except one?

+++ b/simplehtmldom.module
@@ -7,6 +7,8 @@
+define('SIMPLEHTMLDOM_DOWNLOAD_URI', 'https://github.com/konstantin-komelin/simplehtmldom/archive/master.zip');

Is this path to zip file permanent?

alexander_danilenko’s picture

alexander_danilenko commented

But looks good, works good.
+1 to RTBC

Konstantin Komelin’s picture

Konstantin Komelin commented

Thanks @Chernous_dn for the patch. Please give me some time to review it.

Thank you @danilenko_dn for your comments. Let me answer them one by one.

Path hardcoded? What if user will prefer to use multisiting?

1) The path 'sites/all/libraries' is available for all multisite instances.
2) People who configured multisite probably know where to put libraries in their particular cases so we don't have to explain to them.

Is there no other way to exclude all files except one?

I've no idea. If you find a better way please let us know.

https://github.com/konstantin-komelin/simplehtmldom/archive/master.zip
Is this path to zip file permanent?

Yes, absolutely. I created this repo to provide direct and permanent download link because the original repository on Sourceforge doesn't do it.

Thanks,
Konstantin

Konstantin Komelin’s picture

Konstantin Komelin commented
Konstantin Komelin’s picture

Konstantin Komelin commented
Status: Needs review » Needs work

Hi @Chernous_dn,

1) The #15 adds a new line issue in the simplehtmldom.install.
Please install Dreditor to quickly review patches right on your browser https://dreditor.org/

2) Why do we need one more check for libraries module if we already have one in _simplhtmldom_get_library_path() ?

  if ($phase == 'runtime') {
    if (module_exists('libraries') && function_exists('libraries_detect')) {
      $library = libraries_detect('simplehtmldom');

3) Why do we need one more file_exists() check if _simplhtmldom_get_library_path() already checks file existence?

      $library_path = _simplhtmldom_get_library_path();

      if (!file_exists(DRUPAL_ROOT . '/' . $library_path)) {

4) Is it possible to do this?

4) If it's possible let's add one more alias for the drush command - 'simplehtmldom'.

Thanks,
Konstantin

Konstantin Komelin’s picture

Konstantin Komelin commented
Assigned: Konstantin Komelin » Unassigned
astonvictor’s picture

astonvictor commented
Status: Needs work » Closed (outdated)

I'm closing it because the issue was created a long time ago without any further steps.

if you still need it then raise a new one.
thanks