The help block uses the default block cache settings, which is DRUPAL_CACHE_PER_ROLE. This results in the same help block content being displayed on every page (different content for each role), or likely an empty help block on every page. $blocks['help']['cache'] should be either DRUPAL_CACHE_PER_ROLE | DRUPAL_CACHE_PER_PAGE or DRUPAL_NO_CACHE.

CommentFileSizeAuthor
#19 1016582-system-help.patch3.12 KBmfb
#17 1016582-system-help.patch3.04 KBmfb
#16 1016582-system-help-test-only.patch2.6 KBmfb
#14 1016582-system-help.13.test-only.patch3.04 KBsun
#13 1016582-system-help.patch3.48 KBmfb
#12 1016582-system-help.patch2.11 KBmfb
#1 1016582-system-help.patch623 bytesmfb

Comments

mfb’s picture

mfb
they or he
commented
Status: Active » Needs review
StatusFileSize
new 1016582-system-help.patch623 bytes

DRUPAL_NO_CACHE makes sense to me, to avoid a glut of cache entries.

rschwab’s picture

rschwab commented

Can you provide steps to reproduce?

I'm seeing different help block content on different pages, for example at admin/structure/block I see:
"This page provides a drag-and-drop interface for assigning a block to a region, and for controlling the order of blocks within regions." etc etc

And on admin/structure/block/add I see:
"Use this page to create a new custom block."

mfb’s picture

mfb
they or he
commented

Is block caching active for you? Must be enabled and you cannot be uid 1

rschwab’s picture

rschwab commented

Yeah, with block caching enabled and using uid 11 (who has administrator role) I still see the correct help text as described above.

Was there a particular page you noticed this on?

mfb’s picture

mfb
they or he
commented

I can reproduce this bug on any page. Can you run this query to see what's in your cache_block table? Mine has empty string cached as the content so the help block is always empty:

mysql> SELECT cid, data FROM cache_block WHERE cid LIKE 'system:help:%';
+------------------------------+---------------------------------------------+
| cid                          | data                                        |
+------------------------------+---------------------------------------------+
| system:help:mytheme:en:r.1   | a:2:{s:7:"subject";N;s:7:"content";s:0:"";} |
| system:help:mytheme:en:r.2,4 | a:2:{s:7:"subject";N;s:7:"content";s:0:"";} |
| system:help:seven:en:r.2,4   | a:2:{s:7:"subject";N;s:7:"content";s:0:"";} |
+------------------------------+---------------------------------------------+
catch’s picture

catch
he/him
Primary language English
commented
Version: 7.0 » 8.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix, +Needs backport to D7

This looks like just an oversight to me, I can't see it regressing so I'd support it going in without tests.

catch’s picture

catch
he/him
Primary language English
commented

#1: 1016582-system-help.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looks also ready for me. However, writing a help.test for this is a matter of 15 minutes.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Major » Normal

Sorry for the noise. Also not major, just a stupid bug. Use http://api.drupal.org/api/drupal/modules--block--block.api.php/function/... to workaround it.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review

I found something interesting about this bug, which explains why it's not (always) reproducible.

There are two ways for blocks to be added to the block table: by the install profile, or by the block module. The "minimal" and "standard" install profiles included with core both create the system help block with caching disabled! So, it's not easy to create a failing test, since the system help block is already working correctly in a default installation.

I'm sure it's possible to write a test but will take me more than 15 minutes (I already spent 45 minutes figuring out that the block is actually created by the install profile...) Therefore I'd like to nominate this bugfix to be committed without a test :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
  protected $profile = 'testing';

  function setUp() {
    parent::setUp();
    // Delete help block record, so it can be recreated it manually in this test.
    db_delete('block')
      ->condition('module', 'system')
      ->condition('delta', 'help')
      ->execute();
  }
mfb’s picture

mfb
they or he
commented
StatusFileSize
new 1016582-system-help.patch2.11 KB

Ok here's a test which fails without the patch, I guess it just took another 15 minutes..

mfb’s picture

mfb
they or he
commented
StatusFileSize
new 1016582-system-help.patch3.48 KB

The test in this new, improved patch runs many times faster, and actually tests the bug "correctly" by using the testing profile :) yay for testing profile, we can both speed up the test, and test a bug which isn't found in the standard install profile!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new 1016582-system-help.13.test-only.patch3.04 KB

1) TDD: Always confirm that a test catches a bug when fixing bugs.

2) In a final follow-up patch, I think we should be able find a different admin page that contains a shorter help text.

Status: Needs review » Needs work

The last submitted patch, 1016582-system-help.13.test-only.patch, failed testing.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new 1016582-system-help-test-only.patch2.6 KB

Or just assertNoText?

mfb’s picture

mfb
they or he
commented
StatusFileSize
new 1016582-system-help.patch3.04 KB

assertNoRaw that is

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

mmm, no, assertNoText/Raw() is insufficient. The assertion may pass in many situations we can't think of. Assertions should always follow the natural expression of an expectation. Our expectation is "that a different help text appears", not "that a certain help text does not appear on other pages".

mfb’s picture

mfb
they or he
commented
StatusFileSize
new 1016582-system-help.patch3.12 KB

Ok then use a different help block text. although, that's actually how I found this bug, the same help text appeared on every page..

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thank you! :)

webchick’s picture

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

Ugh, that's a crappy bug! Thanks for the fix, and the test!

Committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Needs tests, -Needs backport to D7

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

Issue tags: +Quick fix, +Needs tests, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.