Improve module description and error message about missing Curl library

I think no need to change the hook_help part as if curl is not available then module will not install and so there will be no help page. Help page only exists if module is enabled and if module is enabled, means curl is there.

Was also my first opinion about hook_help. In a site builder point of view, in D7, the right place was README.txt, but does not seem to be popular in D8, that's why I reconsidered hook_help. That's my first patch, any idea why the test does not pass? The text format seems to be OK. Thanks.

cURL is a requirement for other modules (e.g. aggregator). The description of this requirement is duplicated on the hook_requirements of each module, e.g.

$requirements['curl']['description'] = t('The testing framework could not be installed because the PHP <a href="@curl_url">cURL</a> library is not available.', array('@curl_url' => 'http://php.net/manual/curl.setup.php'));

The cURL status is also available on admin/reports/status.
Shouldn't we create + include the URL of a centralized community documentation page about that?
This page could be easily improved over time (when os changes, ...).

@colorfield, I do not know why the automatic test fail but I took your changes and re-roll this patch.

Rebased https://www.drupal.org/node/2474017#comment-9988811 without conflicts. Amended the message a bit to include command line instructions.

Updated issue summary.

+++ b/core/modules/simpletest/simpletest.module
@@ -28,6 +28,9 @@ function simpletest_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<h3>' . t('Requirements') . '</h3>';
+      $output .= '<p>' . t('The PHP <a href="@curl_url">cURL</a> library needs to be installed on the server. You should check the cURL status in PHP via phpinfo() or in the command line via `php -m | grep curl`. On Unix, you have to install the php5-curl extension. On Windows XAMPP, you have to enable the cURL extension by removing the ";" before "extension=php_curl.dll". Apache/nginx must be restarted after applying changes to the server.', array('@curl_url' => 'http://php.net/manual/curl.setup.php')) . '</p>';
+

This help will not be visible unless the module is enabled, and the module can't be enabled without cURL. Can we move this to a drupal.org page and link it from the module description?

Agree with #20. Also I'm not sure about whether we should include this in core. If this is going to be part of core then we have to find some way to invoke hook_requirements() while generating the module list page. Currently it only invokes while a module is installed. I pinged my concerns in IRC but nobody replied. I will raise my concerns again. And definitely this is not a novice issue. Also the issue summary seems misleading if we are not going to insert the requirements in hook_help() (we need to correct the screenshots).

Apparently this needs to be "rc deadline" because it changes translatable UI text strings. See https://groups.drupal.org/node/484788

I believe testbot has curl extension enabled by default, therefore marking this as "Needs manual testing" because someone has to test whether this "actually" works. I don't have a clue how to disable curl extension in MAMP. Will try to test this in my Ubuntu machine later.

+++ b/core/modules/simpletest/simpletest.module
@@ -27,6 +27,7 @@ function simpletest_help($route_name, RouteMatchInterface $route_match) {
+

oops..

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -419,6 +419,18 @@ protected function buildModuleList(FormStateInterface $form_state) {
   +    $form_state_values = $form_state->getValues();
   +    $has_curl = function_exists('curl_init');
   

   can we use extension_loaded() instead of function_exist() ?

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -419,6 +419,18 @@ protected function buildModuleList(FormStateInterface $form_state) {
   +    if (!$has_curl && $form_state_values['modules']['Core']['simpletest'] === 1) {
   +      $form_state->setError('modules', t('The PHP <a href="@curl_url">cURL</a> library needs to be installed on the server. You should check the cURL status in PHP via phpinfo() or in the command line via `php -m | grep curl`. On Unix, you have to install the php5-curl extension. On Windows XAMPP, you have to enable the cURL extension by removing the ";" before "extension=php_curl.dll". Apache/nginx must be restarted after applying changes to the server.', array('@curl_url' => 'http://php.net/manual/curl.setup.php')));
   +    }
   

   $this->t()

@joshi.rohit100 Thanks for the review. I am not sure about your first suggestion. There's an issue for this #1942432: Use extension_loaded instead of function_exists to detect PHP extensions, the comment [#1942432.4] is quite interesting and is the reason why I don't want to use extension_loaded(). Also I took the code from simpletest_requirements(), I am inclined towards function_exists(). Can you please test this patch.

Thanks!

So... At this point, except for dire emergencies, we cannot change translated strings. So we don't want to change the module description as is done in the patch (the part in the .info.yml file). Besides which, there are zero other module descriptions in Drupal Core that say what the requirements are for installing the modules -- that is not how we indicate that -- we use hook_requirements().

The other part of the patch looks wrong to me too. It seems to be requiring that the curl library be installed for ***any*** module installation. Uh???? That cannot be right. We only want to improve the feedback for the simpletest module.

So this patch is not OK.

What we would *actually* want to do is modify simpletest_requirements(), which is the hook_requirements() implementation in core/modules/simpletest/simpletest.install. That is already testing whether curl is installed. This issue, at least according to the title, is about improving the message that you get if the curl library is not installed, which is generated inside that function.

And... at this point, we are past Release Candidate, so fixing that message would probably require a translated string change. I do not think that a "normal" bug is enough to justify that. So moving this issue to 8.1 at this point.

These supposed patch files have all kinds of weird characters in them. I'm inclined to think they are spam and am reporting them as such.

@jhodgdon I have deleted the Patch,Thanks for letting me know

What were those files???

In conclusion, I guess :
- As we should not say what the requirements are for installing the module in the module description.
- As the help text will not be visible unless the module is enabled.
- As we can't disable the ability to enable a module in module list if a requirement (other than the presence of a module) is not met.

=> I just applied the new error message in hook_requirements, until we have a documentation page on Drupal.org.

A few suggestions on this text:

a) See #2571845: Links to wikipedia/php.net should be part of the actual t() string so they can be localized -- we actually want php.net and wikipedia links to be part of the t() string, not separated in options, in translation strings.

b) Can we make the link text for the php.net link "PHP cURL library" for better accessibility?

c) "You should check the cURL status in PHP via phpinfo() or in the command line via `php -m | grep curl`. "

We don't normally want to say "should", maybe "can". But... if this text is being shown, doesn't that mean cURL is not enabled? In which case, why would they even need to check the status of cURL?

Anyway, if for some reason you do think we need this sentence, let's link them to the phpinfo() report within Drupal, the one that the route "system.php", rather than suggesting running commands at the command line or within PHP, which the average site builder would not know how to run.

d) Maybe prefix the installation instructions with something like "To install the cURL library..."?

This patch is applying #42 suggestions.

c) I do not know if we need the sentence, anyway I made the link to the phpinfo() report page, I choosed "phpinfo() page" for the link name.

Much better, thanks! Removing some extraneous tags...

So... I have a few questions that I don't know the answer to:

a) I am not sure we should say "Unix". Most people do not run Unix per se, and it is a trademark. We should see what we do in other readme files and documentation to indicate "something like Unix or Linux or whatever" and do the same as we've done there.

b) Do you think people would know how to install PHP extensions on Linux/Unix?

c) Do the Unix/Linux instructions also apply to Mac?

d) The Windows instructions say you need to remove a ; somewhere but don't say what file that is.

e) In general... I am not sure we should be putting all of this information into a hook_requirements(). I think normally what we do is put a short message and link to a drupal.org page that gives more information, don't we?

I think we only need to say "you need the curl extension". That is done in a myriad of different ways on different operating systems and development stacks, so I think trying to cover common use cases in help text is going to be difficult, and potentially confuse people when, for example, they are on Windows and don't use XAMPP.

Well... The main point of this issue is that in simpletest.module (as well as aggregator.module and maybe elsewhere?), we are currently just linking people to http://php.net/manual/en/curl.setup.php . This page has zero information on it. If you click through to the Installation page you get to http://php.net/manual/en/curl.installation.php -- and that page tells you to recompile PHP (which I think nearly no one will be doing).

But I don't think we can just say "you need cURL" without some link to how to install it. That wouldn't be very illuminating either, though probably slightly better than linking them to a page that says they need to recompile PHP.

So - can we start a docs page with a nice URL on d.o? I can begin writing up installation procedures for common platforms and dev packages.

Probably there should be a brief mention in this section along with the other extensions:
https://www.drupal.org/requirements/php#extensions

Then you can make a child page for the details, and if you start one I can add a nice URL to it. Thanks!

Ok, mentioned here: https://www.drupal.org/requirements/php#curl

Child page: https://www.drupal.org/node/2697465

Looks good. I've added an alias for the page, so you can now see it at
https://www.drupal.org/requirements/php/curl

Question about the page: it seems like it is saying at the top that cURL is required for Drupal 8 in general... that isn't right, is it? Must be the wording. Also, another note: the aggregator module also requires cURL at least in 8, possibly also in 7.

And... THANKS!

Ah yes, you're right, I misinterpreted the Guzzle requirements as needing cURL when that's not a hard requirement per: http://guzzle.readthedocs.org/en/latest/overview.html#requirements

Core seems to be using Guzzle 6.1.0 so PHP stream wrappers are an option. Let me update the docs and roll a new patch pointing at them.

Thanks! I did a minor edit at the top of the docs page (the simpletest module is actually called Testing in the UI, and also I mentioned Aggregator).

Regarding the patch:

a) Don't say "please". See
https://www.drupal.org/node/604342
(Style section at the top).

b) We need this in aggregator module as well as simpletest.

Other than that, looks good!

Thanks!

Updated:

• Changed simpletest install message to use the same pattern as the following and preceding statements
• Changed "you can check the cURL status" to "you can check your PHP configuration"
• Added the notice to aggregator module.

Looks great to me! One small thing:

+++ b/core/modules/simpletest/simpletest.install
@@ -6,6 +6,7 @@
+use Drupal\Core\Url;

This "use" line is missing from aggregator.install

I have not pursued cause I was sailing !

I added the "use" instruction, and I guess the link to "Install the PHP cURL extension" page needed a name.

Almost!

1. +++ b/core/modules/aggregator/aggregator.install
   @@ -17,7 +19,7 @@ function aggregator_requirements($phase) {
   +    $requirements['curl']['description'] = t('The Aggregator module could not be installed because the PHP <a href="http://php.net/manual/curl.setup.php">cURL</a> library is not available. You can check your PHP configuration via the <a href="@phpinfo_url">phpinfo() page</a>. For instructions on how install the cURL library, see <a href="https://www.drupal.org/requirements/php/curl">Install the PHP cURL extension</a>.', array('@phpinfo_url' => (new Url('system.php'))->toString()));
   

   Can we change the wording of the first sentence here to be more similar to the other one? Something like:

   The Aggregator module requres the (link)PHP cURL library(end of link).

2. +++ b/core/modules/simpletest/simpletest.install
   @@ -27,7 +28,7 @@ function simpletest_requirements($phase) {
   +    $requirements['curl']['description'] = t('The testing framework requires the <a href="http://php.net/manual/curl.setup.php">PHP cURL library</a>. You can check your PHP configuration via the <a href="@phpinfo_url">phpinfo() page</a>. For instructions on how install the cURL library, see <a href="https://www.drupal.org/requirements/php/curl">https://www.drupal.org/requirements/php/curl</a>.', array('@phpinfo_url' => (new Url('system.php'))->toString()));
   

   The last "For instructions on how to install..." sentence here didn't get the updates you made to the Aggregator sentence (that one seems better to me).

Both messages should be consistent now, with the only differences the name of the module being enabled (or not, in this case).

Looks good now, thanks!

Updated issue summary.

If you're following this issue and have instructions for another platform, please add them to https://www.drupal.org/requirements/php/curl :)

This makes the message very long, and it's possible that people won't be able to install curl themselves. What about linking to https://www.drupal.org/requirements/php/curl and putting all the relevant information there?

The main problem I see with the suggestion in #61 is that the drupal.org page will not be translated, and a secondary problem is that it is not included with the Drupal.org download.

So I think we need to provide at least the basics in the message rather than assuming that (a) the drupal.org page is reachable and (b) it is readable by everyone who needs the information (such as Chinese readers). I think we have taken that stance before in UI text and help: provide the bare minimum in the UI text, and provide a link to drupal.org or php.net or Wikipedia or whatever for more information. The question is what "bare minimum" is... we want to make sure that whatever we provide is clear and understandable even if drupal.org is not readable for the user.

I expected to find this on the Extend page but don't see anything changed there using simplytest. Should I be looking elsewhere?

As noted in the issue summary, you only get the message when you actually go to try to install either Aggregator or Testing module, and then only if your server is missing the cURL library.

Ah right, my apologies. But, that makes the UI hard to review indeed. Hope somebody can add some screenshots.

Indeed. I think you can temporarily disable the cURL extension from PHP on a local server in order to see it... but not sure. Also the original issue asked that this be improved by adding it to the Extend page, but the problem is that it's hook_requirements() not a module depending on another module, and we have no easy way to do that on the Extend page at this time. Perhaps we should rethink that.

Can we get screenshots, this is not reviewable :(

I guess I'm too brutal ? This is what I do to test it :

function simpletest_requirements($phase) {
  $requirements = array();

  $has_curl = false;//function_exists('curl_init');

And here is a screenshot :

try-install-after-patch

Links check :

• "PHP cURL library" link goes to http://php.net/manual/fr/curl.setup.php
• "phpinfo() page" link goes to /admin/reports/status/php
• "Install the PHP cURL extension" link goes to https://www.drupal.org/requirements/php/curl

I'm at DrupalCon and am about to try and test this fix...

Thanks for the screenshots in #68 -- that does seem like an excellent way to test the UI. Any usability comments?

Thanks for the ongoing work to refine this text. I agree that a hook_requirements() message is more appropriate than the module description.

+++ b/core/modules/aggregator/aggregator.install
@@ -17,7 +19,7 @@ function aggregator_requirements($phase) {
-    $requirements['curl']['description'] = t('The Aggregator module could not be installed because the PHP <a href="http://php.net/manual/curl.setup.php">cURL</a> library is not available.');
+    $requirements['curl']['description'] = t('The Aggregator module requires the <a href="http://php.net/manual/curl.setup.php">PHP cURL library</a>. You can check your PHP configuration via the <a href="@phpinfo_url">phpinfo() page</a>. For instructions on how install the cURL library, see <a href="https://www.drupal.org/requirements/php/curl">Install the PHP cURL extension</a>.', array('@phpinfo_url' => (new Url('system.php'))->toString()));
   }

diff --git a/core/modules/simpletest/simpletest.install b/core/modules/simpletest/simpletest.install
index e5eb9a3..9ebbb40 100644

+++ b/core/modules/simpletest/simpletest.install
@@ -27,7 +28,7 @@ function simpletest_requirements($phase) {
-    $requirements['curl']['description'] = t('The testing framework could not be installed because the PHP <a href="http://php.net/manual/curl.setup.php">cURL</a> library is not available.');
+    $requirements['curl']['description'] = t('The testing framework requires the <a href="http://php.net/manual/curl.setup.php">PHP cURL library</a>. You can check your PHP configuration via the <a href="@phpinfo_url">phpinfo() page</a>. For instructions on how install the cURL library, see <a href="https://www.drupal.org/requirements/php/curl">Install the PHP cURL extension</a>.', array('@phpinfo_url' => (new Url('system.php'))->toString()));

These strings should use the URL placeholders (:phpinfo_url rather than @phpinfo_url).

Also, I think it would probably be better and less verbose to use \Drupal::url(). (I realize HEAD already has both patterns for system.php.) That would also mean removing the use statements. :)

Finally, I'm wondering if this text is actually too much information. I see that @catch and others have given the same feedback. We are providing three different actions for the user to follow. Do we need all three of them? Would simply the second two (the PHP info page and the comprehensive d.o docs) be sufficient, now that we have that handbook page? Or something more concise like:

The testing framework <a href="https://www.drupal.org/requirements/php/curl">requires the PHP cURL extension</a>.

Because the first two times I read it, I thought that "Install the PHP cURL extension" was somehow a link to do that, rather than a d.o handbook page about it.

Applied the 2 first points of #74 at the moment.
So missing the text simplification part, cause I don't know.

Just changed a typo in the title

Thanks @ifrik for the clarification, as I was not sure to make it right.
Applied #77 suggestion.

Re-rolled patch from #78 so php.net urls use https.

...

For what is worth, this is how it looks #79 patch in 8.3.x branch (also looks good in 8.2.x)

+++ b/core/modules/aggregator/aggregator.install
@@ -17,7 +17,7 @@ function aggregator_requirements($phase) {
+    $requirements['curl']['description'] = t('The Aggregator module requires the <a href="https://secure.php.net/manual/en/curl.setup.php">PHP cURL library</a>. For more information, see the <a href="https://www.drupal.org/requirements/php/curl">online information on installing the PHP cURL extension</a>.', array(':phpinfo_url' => \Drupal::url('system.php')));

+++ b/core/modules/simpletest/simpletest.install
@@ -27,7 +27,7 @@ function simpletest_requirements($phase) {
+    $requirements['curl']['description'] = t('The testing framework requires the <a href="https://secure.php.net/manual/en/curl.setup.php">PHP cURL library</a>. For more information, see the <a href="https://www.drupal.org/requirements/php/curl">online information on installing the PHP cURL extension</a>.', array(':phpinfo_url' => \Drupal::url('system.php')));

The phpinfo URL is not used in either text.

I have removed the phpinfo URL from the patch #79

Looks good now and the direction is in line with the discussion above.

Committed 102175d and pushed to 8.3.x. Thanks!

Yay. thanks all!