Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | simplehtmldom-implement-drush-command-for-installing-simplehtmldom-library-2199493-15.patch | 8.44 KB | Chernous_dn |
Comments
Comment #1
Nikolay Shapovalov+1
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedFailing 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.
Comment #3
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedThank 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.
Comment #4
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedScheduled this task to DrupalCon Amsterdam.
Comment #5
Gábor HojtsyLet's use Amsterdam2014 tag, thanks!
Comment #6
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedSure. Thank you Gábor!
Comment #7
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedComment #8
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedComment #9
Chernous_dn CreditAttribution: Chernous_dn commentedI created drush command, to download library after enable module.
Comment #10
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedThanks @Chernous_dn for your patch. I will review it as soon as possible.
Comment #11
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedComment #12
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedHi @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
Comment #13
Chernous_dn CreditAttribution: Chernous_dn commentedI made a mistake when creating a patch and forgot to test. Вut now I have created a proper patch and tested it.
Comment #14
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedHi @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:
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:
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
should be:
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
Comment #15
Chernous_dn CreditAttribution: Chernous_dn commentedThis patch all of your comments and recommendations. This patch covers all questions in the previous comments.
Comment #16
alexander_danilenko CreditAttribution: alexander_danilenko commentedPath hardcoded? What if user will prefer to use multisiting?
Is there no other way to exclude all files except one?
Is this path to zip file permanent?
Comment #17
alexander_danilenko CreditAttribution: alexander_danilenko commentedBut looks good, works good.
+1 to RTBC
Comment #18
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedThanks @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.
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.
I've no idea. If you find a better way please let us know.
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
Comment #19
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedComment #20
Konstantin Komelin CreditAttribution: Konstantin Komelin commentedHi @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() ?
3) Why do we need one more file_exists() check if _simplhtmldom_get_library_path() already checks file existence?
4) Is it possible to do this?
Thanks,
Konstantin
Comment #21
Konstantin Komelin CreditAttribution: Konstantin Komelin commented