If the PHP memory limit is lower than recommended the Status report page (admin/reports/status) currently only directs you to the Requirements page (drupal.org/requirements). With the patch I attached the notice also displays a link to the handbook page on increasing you memory limit (drupal.org/node/207036). As this handbook page is quite useful and covers all different options for different types of users, this is to my mind a small but nice usability improvement.

CommentFileSizeAuthor
#20 check-memory-limit-343706-20.patch2.25 KBdrupal_was_my_past
#16 343706_check_memory_limit[2].patch2.55 KBtstoeckler
#14 343706_check_memory_limit.patch2.53 KBdeviantintegral
#8 check_php_mem_limit_03.patch1 KBtstoeckler
#5 check_php_mem_limit_03.patch1 KBtstoeckler
#3 check_php_mem_limit_02.patch1.14 KBtstoeckler
check_php_mem_limit.patch1.14 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review

Sorry, wrong status...

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
check_php_mem_limit_02.patch1.14 KB

Wrong range in the chunk of the patch. Hope this works...

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
check_php_mem_limit_03.patch1 KB

One last try.... (poor testbot, just because I can't roll a patch properly...)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Yay!

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
check_php_mem_limit_03.patch1 KB

retry...

lambic’s picture

lambic CreditAttribution: lambic commented
Status: Needs work » Needs review
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

This looks good to me. It's been suggested that a test for the memory limit requirement would be useful, but I think that could be handled in a subsequent patch. +1 committing #8.

roychri’s picture

roychri CreditAttribution: roychri commented
Status: Needs review » Reviewed & tested by the community

I had problems trying to test this.

The system.module contains the constant that defines what the minimum memory limit should be. It is currently set to 16M.
First I made my memory limit to 15M and tried installing drupal and the requirements verification did not complain about it.
The system_requirements() is considering the memory limit requirements to be a warning when it is not met.
I suspected that maybe the system would not care if only warnings were detected.
@deviantintegral confirmed that for me.

So, I tested this by upping the php version requirement to 5.9.0 (line 22 from system.module) in order to trigger an error from the install so I can see the warning.

This is working for me and I think this is rtbc.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

Which makes me think... a new issue for throwing an error if the memory is < 16MB, a warning for 16.1-32, pass for 32+?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Category: feature » task
Status: Reviewed & tested by the community » Needs work

I definitely like this change. But let's massage that string a little:

"See the Drupal requirements or the online handbook entry for increasing the PHP memory limit for more information."

1. I'd say the instructions for resolving the issue are way more important than Drupal's list of system requirements. I'd definitely put it first.
2. However, since the requirements list is about more than just the memory requirement, what if we moved the link to the requirements outside of this one error message to somewhere else in the installer error page that's more 'global'?
3. Then, we could shorten this string to something like "You need to increase PHP's memory limit in php.ini. Learn more" (make it consistent with other error messages on that screen).

Also, "@php_memory_limit-handbook" looks funny with the mixed dashes/underscores, and is also really long. The standard in core seems to be picking one-word descriptions for these placeholders. How about @requirements and @documentation, or just @url if we remove the requirements link?

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Status: Needs work » Needs review
FileSize
343706_check_memory_limit.patch2.53 KB

Here's an update that:

  • Shortens the memory limit text to For more information, see the online handbook entry for <a href="@memory-limit">increasing the PHP memory limit</a>. This text is very similar to the Cron error text.
  • Adds Before filing a support request, ensure that your web server meets <a href=\"@system-requirements\">Drupal's system requirements</a>. to the top of the intro paragraph for the status page.
cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

#14: 343706_check_memory_limit.patch queued for re-testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
343706_check_memory_limit[2].patch2.55 KB

I think I remember there was some kind of standard of Drupal not referring to itself, ie not calling itself "Drupal". I manually edited the patch file, which now contains the string:
Before filing a support request, ensure that your web server meets the <a href=\"@system-requirements\">system requirements</a>.
Otherwise, I'd say #14 is RTBC (didn't manually test it though. But if no tests fail, and the strings are good...)

Status: Needs review » Needs work

The last submitted patch, 343706_check_memory_limit[2].patch, failed testing.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

I'm pretty sure Drupal is fine to use in strings; currently it's used in over 100 strings.

yoroy’s picture

yoroy CreditAttribution: yoroy commented
Version: 7.x-dev » 8.x-dev

Actually, it's discouraged to use Drupal, it complicates custom distributions, and well, the Drupal context is ususally a given.

drupal_was_my_past’s picture

drupal_was_my_past CreditAttribution: drupal_was_my_past commented
Status: Needs work » Needs review
FileSize
check-memory-limit-343706-20.patch2.25 KB

Re-rolled patch from #16.

drupal_was_my_past’s picture

drupal_was_my_past CreditAttribution: drupal_was_my_past commented
Assigned: Unassigned » drupal_was_my_past
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Status: Needs review » Reviewed & tested by the community

Patch in #20 looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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