Updated: Comment #N

Problem/Motivation

Adding a base theme to info.yml file gives this warning, e.g.

base theme: bartik

Full warning message:

Warning: Illegal offset type in isset or empty in Drupal\Core\Extension\ModuleHandler->loadInclude() (line 193 of core/lib/Drupal/Core/Extension/ModuleHandler.php).

Proposed resolution

Fix views.
Add test for it.

Remaining tasks

User interface changes

API changes

Original report by @chris_hall_hu_cheng

Adding a base theme to info.yml file gives this warning, e.g.

base theme: bartik

Full warning message:

Warning: Illegal offset type in isset or empty in Drupal\Core\Extension\ModuleHandler->loadInclude() (line 193 of core/lib/Drupal/Core/Extension/ModuleHandler.php).

Files: 
CommentFileSizeAuthor
#102 interdiff-2084907-78-102.txt3.47 KBchris_hall_hu_cheng
#102 core-views-2084907-102-fail.patch4.96 KBchris_hall_hu_cheng
FAILED: [[SimpleTest]]: [MySQL] 63,203 pass(es), 2 fail(s), and 6 exception(s). View
#102 core-views-2084907-102.patch6.28 KBchris_hall_hu_cheng
PASSED: [[SimpleTest]]: [MySQL] 63,703 pass(es). View
#97 core-views-2084907-97--fail.patch4.96 KBNoe_
FAILED: [[SimpleTest]]: [MySQL] 63,128 pass(es), 4 fail(s), and 7 exception(s). View
#92 interdiff-2084907-78-89.txt3.46 KBchris_hall_hu_cheng
#89 core-views-2084907-89.patch6.28 KBchris_hall_hu_cheng
PASSED: [[SimpleTest]]: [MySQL] 59,897 pass(es). View
#78 core-views-2084907-78.patch6.7 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,938 pass(es). View
#75 core-views-2084907-75.patch6.65 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,448 pass(es). View
#68 core-views-2084907-68.patch6.82 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,328 pass(es). View
#64 core-views-2084907-64-should-fail.patch3.42 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 59,323 pass(es), 0 fail(s), and 6 exception(s). View
#60 core-views-2084907-60-should-fail.patch2.6 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,338 pass(es). View
#60 core-views-2084907-60.patch3.87 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 59,294 pass(es), 1 fail(s), and 0 exception(s). View
#57 core-views-2084907-55.patch2.88 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 59,573 pass(es), 2 fail(s), and 0 exception(s). View
#51 drupal_2084907_51.patch1.27 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 60,059 pass(es). View
#34 drupal_2084907_34.patch1.15 KBdiscipolo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2084907_34.patch. Unable to apply patch. See the log in the details link for more information. View
#24 drupal_2084907_24.patch1.32 KBXano
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2084907_24.patch. Unable to apply patch. See the log in the details link for more information. View
#4 loadinclude-illegal-offset-type-2084907-4.patch1.15 KBchris_hall_hu_cheng
PASSED: [[SimpleTest]]: [MySQL] 59,048 pass(es). View

Comments

Jeff Burnz’s picture

Component: base system » views.module

I found this only happens on pages generated with Views.

chris_hall_hu_cheng’s picture

I have the same bug and under the same conditions.

The warning occurs after activating a sub-theme of Bartik. If I get a chance later will try to debug.

cpj’s picture

I get the same error too.

The first argument to Drupal\Core\Extension\ModuleHandler->loadInclude() is $module, which is normally the name of a module. Line 192 (in the version of 8.x that I have) uses this as an array key to check if the module exists in the module list array with

    if (isset($this->moduleList[$module])).

In the case of a sub-theme, the issue is that Drupal\views\ViewExecutable->render() line 1367 does

    // Let the themes play too, because pre render is a very themey thing.
    foreach ($GLOBALS['base_theme_info'] as $base) {
      $module_handler->invoke($base, 'views_pre_render', array($this));
    }

which then calls implementsHook(), which calls loadInclude(). Instead of passing the name of a module, Drupal\views\ViewExecutable->render passes the stdClass for the parent theme object in $base. This is eventually passed into loadInclude() as $module, so, the isset() fails because it is expecting a string or an integer as the array key, not an object.

chris_hall_hu_cheng’s picture

Assigned: Unassigned » chris_hall_hu_cheng
Status: Active » Needs review
FileSize
1.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,048 pass(es). View

Well, when I came back to this, turns out cpj had already done the debugging bit.

There is another invocation of view_post_render a little later in the code.

The attached patch fixes by changing $base to $base->name. ie:

 // Let the themes play too, because pre render is a very themey thing.
      foreach ($GLOBALS['base_theme_info'] as $base) {
        $module_handler->invoke($base->name, 'views_pre_render', array($this));
      }

      $module_handler->invoke($GLOBALS['theme'], 'views_pre_render', array($this));

      $this->display_handler->output = $this->display_handler->render();
      if ($cache) {
        $cache->cacheSet('output');
      }
    }

    $exposed_form->postRender($this->display_handler->output);

    if ($cache) {
      $cache->postRender($this->display_handler->output);
    }

    // Let modules modify the view output after it is rendered.
    $module_handler->invokeAll('views_post_render', array($this, &$this->display_handler->output, $cache));

    // Let the themes play too, because post render is a very themey thing.
    foreach ($GLOBALS['base_theme_info'] as $base) {
      $module_handler->invoke($base->name, 'views_post_render', array($this));
    }

If this actually gets committed I would say cpj deserves a credit also.

Jeff Burnz’s picture

Patch in #4 is working for me.

cpj’s picture

Yes, that works. Drupal\Core\Extension\ModuleHandler->loadInclude() is now passed a string (the parent theme name), so the isset works correctly and returns false as expected (the parent theme name isn't the name of a module)

chris_hall_hu_cheng’s picture

@cpj

I believe (but haven't checked this yet) that in theory the invocation on the theme and parent theme that Views is calling would work if both/either had implemented the hook called in their .theme files. Similar to how it used to work in template files. The themes would then appear in the module handler as 'honary' modules. I am going to test all this when I get the time though and write it up. If this is not how it works in 8 then technically the Views module is wasting time with these calls anyway but at least the patch prevents it from wasting time and throwing an error.

There is (or has been in the past) a blurred line between themes and modules in some areas and I am not sure how blurred this is now, particularly as this involves hooks which have had lost some of their power in D8

The challenge now is to get the patch committed or at least get meaningful feedback on why it shouldn't be....

cpj’s picture

@chris_hall_hu_cheng

Passing an object rather than a string was incorrect, so even if there is further possible optimization in the Views module, right now this patch is needed to make sub-themes work at all, and therefore it should be committed.

cpj’s picture

Issue summary: View changes

Remove spelling mistake.

derheap’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
derheap’s picture

I just tested the patch and looked at it.
I can spot no error in the two line change.
And it works for me, so I changed it to RTBC.

derheap’s picture

Issue summary: View changes
mermentau’s picture

Patch works on latest Drupal 8 dev and AdaptiveTheme 8.x-1.x-dev.

Xano’s picture

chris_hall_hu_cheng’s picture

chris_hall_hu_cheng’s picture

I think I will re-visit and re-queue the patch for re-testing every few days, that way I will eventually know roughly when it fails due to commits elsewhere and perhaps have time to re-do. unless this is considered bad form?

chris_hall_hu_cheng’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: loadinclude-illegal-offset-type-2084907-4.patch, failed testing.

chris_hall_hu_cheng’s picture

Aha failed testing, so I guess I now have to find out why, fix it for the latest dev 8 (if the problem has not been fixed elsewhere) re-submit the patch and hope the "The community" has time to test it. Good job it is not a complicated change really.

Sadly I don't have enough spare time for at least a week.

I guess there is a chance some other factor broke the test, so maybe I will submit it again in a day or so (I am feeling rather lazy).

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review
chris_hall_hu_cheng’s picture

Status: Needs review » Reviewed & tested by the community

Pushing this back to RTBC as the test failure appears to be a temp testing glitch?

discipolo’s picture

the patch applies and works for zurb_framework subthemes

derheap’s picture

HEAD (c465f9667f8) broke this patch again. Needs a reroll.

derheap’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2084907_24.patch. Unable to apply patch. See the log in the details link for more information. View
derheap’s picture

Status: Needs review » Reviewed & tested by the community

Patch #24 works again.
Still looking good – just a two line change.
Putting back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: drupal_2084907_24.patch, failed testing.

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review

24: drupal_2084907_24.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 24: drupal_2084907_24.patch, failed testing.

Jeff Burnz’s picture

Maybe there is still some instability with the test bots?

chris_hall_hu_cheng’s picture

I am going to test myself against latest head at the weekend, find out what is going on, am on a mission now, only two tiny changes but it does remove a big fat red message when you sub-theme and presumably allows the hook to run on parent themes, would be nice to have it committed and then just forget about it.

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review

24: drupal_2084907_24.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 24: drupal_2084907_24.patch, failed testing.

chris_hall_hu_cheng’s picture

Now it is properly broken :/

discipolo’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2084907_34.patch. Unable to apply patch. See the log in the details link for more information. View

maybe a reroll helps

Status: Needs review » Needs work

The last submitted patch, 34: drupal_2084907_34.patch, failed testing.

chris_hall_hu_cheng’s picture

discipolo’s picture

Maybe the test needs attention? Did you try running FilterUidRevisionTest.php locally? Maybe test bot is in a bad mood.

pfrenssen’s picture

I've applied the patch and have run FilterUidRevisionTest locally and it was green, both using Simpletest from the interface and from run-tests.sh.

discipolo’s picture

Status: Needs work » Needs review

34: drupal_2084907_34.patch queued for re-testing.

pfrenssen’s picture

It's green now, nice bot :)

discipolo’s picture

Status: Needs review » Reviewed & tested by the community

That means this can be committed right?

chris_hall_hu_cheng’s picture

34: drupal_2084907_34.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +VDC

This could use an automated test - looks like there's no coverage of this.

chris_hall_hu_cheng’s picture

Could you give me an example of something similar in nature that has a test so I can get a feel for scope?

I had a simple two line fix for an issue that will affect everybody who tries to make a sub-theme and uses views (posting a big red warning), one and half months later and if feels a step behind where it was initially, I assume if I work in a valid test and add it will the issue need to reviewed by the community again??

Some guidance please on what exactly I would have to do to this issue, or my Drupal profile /activity etc. to expedite and what stages it will have to go through to get committed with realistic timescale for all this to happen.

I assume every issue that needs a patch didn't have test coverage (otherwise the tests would have failed before that point).

John Pitcairn’s picture

Confirming patch at #34 works for me, using a Bartik subtheme.

tYb’s picture

Warning solved with #34 with Bartik subtheme.

timodwhit’s picture

+1 #34 works for me.

Xano’s picture

Status: Needs work » Needs review

34: drupal_2084907_34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 34: drupal_2084907_34.patch, failed testing.

Berdir’s picture

To write a test for this, you need to enable a theme with a base theme in a test and then visit a page that triggers this bug, with views.module enabled, the frontpage should be enough I guess. php notices automatically trigger test exceptions. Upload a patch with a test and

Given that it's a bug in views.module, the test should be in there, maybe ViewsBaseThemeTest or so?

Looks like there's test_basetheme, I guess you could use that in your test.

For bonus points, you could actually test the specific functionality that is broken, implement views_pre_render in the base system and set a state variable or print a message and then check that the code was executed.

clemens.tolboom’s picture

FileSize
1.27 KB
PASSED: [[SimpleTest]]: [MySQL] 60,059 pass(es). View

Rerolled patch.

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review
timodwhit’s picture

Status: Needs review » Reviewed & tested by the community

Tested and Working.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

This has been set back to needs work before due to missing test coverage.

chris_hall_hu_cheng’s picture

And at some point I am happy to look at adding the test/s but currently rammed with work and social commitments ;(

clemens.tolboom’s picture

Assigned: chris_hall_hu_cheng » Unassigned
Status: Needs work » Needs review

I've tried to prepare a test. The attached patch only fails. I digged up some classes with enable themes that's it.

So still needs much work.

clemens.tolboom’s picture

Issue summary: View changes
FileSize
2.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,573 pass(es), 2 fail(s), and 0 exception(s). View

(add patch file)

The frontpage from the test results shows a login so I guess test needs
- a user login or access for anonymous
- maybe a node for the frontpage?
- both test_themebase and test_themesub

Status: Needs review » Needs work

The last submitted patch, 57: core-views-2084907-55.patch, failed testing.

clemens.tolboom’s picture

The tests were meant to fail as not much is tested yet. No harm done :)

Please fill in the holes

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,294 pass(es), 1 fail(s), and 0 exception(s). View
2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,338 pass(es). View

I've managed to fill some holes.

Problem is the fail patch does not fail on my system. Is the subtheme really active? Is there a cache active?

clemens.tolboom’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
@@ -0,0 +1,88 @@
+    // Make base theme default first
+    \Drupal::config('system.theme')
+        ->set('default', 'test_basetheme')
+        ->save();
+    $this->assertEqual(\Drupal::config('system.theme')->get('default'), 'test_basetheme');
+

Move this to just before the first drupalGet('node') as that is run with base theme.

The last submitted patch, 60: core-views-2084907-60.patch, failed testing.

clemens.tolboom’s picture

Failure for test is

Segmentation fault
FATAL Drupal\views\Tests\Handler\FieldWebTest: test runner returned a non-zero error code (139).

As the others passed no need for a retest now as core-views-2084907-60-should-fail.patch should fail first.

clemens.tolboom’s picture

FileSize
4.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,355 pass(es). View
3.42 KB
FAILED: [[SimpleTest]]: [MySQL] 59,323 pass(es), 0 fail(s), and 6 exception(s). View

I found the 'bug' which is #2167347: It's possible to enable a non-existing theme

Attached patches should nail it now. I renamed the class to ViewsThemeIntegrationTest as we probably need more tests later on.

The patch has a theme_subtheme/css/ added to give a visual cue the theme is selected. Feel free to remove but then please create a new issue out of it :)

Status: Needs review » Needs work

The last submitted patch, 64: core-views-2084907-64-should-fail.patch, failed testing.

Berdir’s picture

As suggested above, I would suggest to implement views_pre_render in base theme and e.g. do a drupal_set_message() there. Then you not only cover the notices but the actual bug that they are hinting at.

clemens.tolboom’s picture

@Berdir those where bonus points :p

As #60 did not failed I first needed to fix for that. And that was a time drainer as found by #2167347: It's possible to enable a non-existing theme

Are you OK with the patch so far?

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.92 KB
6.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,328 pass(es). View

Added the hook_views_pre|post_render hooks

As the failure is proven by #60 : core-views-2084907-60-should-fail.patch no new file.

Status: Needs review » Needs work

The last submitted patch, 68: core-views-2084907-68.patch, failed testing.

clemens.tolboom’s picture

The failure is about

Drupal\system\Tests\Common\FormatDateTest->testAdminDefinedFormatDate()

looks completely unrelated. wtf is the testbot doing :-/

So requesting a new test.

clemens.tolboom’s picture

Status: Needs work » Needs review

68: core-views-2084907-68.patch queued for re-testing.

clemens.tolboom’s picture

  1. +++ b/core/modules/system/tests/themes/test_basetheme/test_basetheme.theme
    @@ -0,0 +1,17 @@
    +function test_basetheme_views_pre_render(ViewExecutable $view) {
    

    Needs function documentation:

    /**
     * Implements hook_views_pre_render().
     */
    
  2. +++ b/core/modules/system/tests/themes/test_basetheme/test_basetheme.theme
    @@ -0,0 +1,17 @@
    +function test_basetheme_views_post_render(ViewExecutable $view) {
    

    Needs function documentation:

    /**
     * Implements hook_views_post_render().
     */
    
  3. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    + * Related tests are
    + *
    + * @see Drupal/system/Tests/Theme/ThemeInfoStylesTest
    + * @see Drupal\views\Tests\ViewRenderTest
    + * @see Drupal/node/Tests/Views/FrontpageTest
    + * @see Drupal\system\Tests\Theme\EntityFilteringThemeTest : switching and testing all themes
    + * @see Drupal\system\Tests\Theme\ThemeInfoStylesTest : dumps css sub theme changes
    

    Rephrase or delete?

  4. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    +/**
    ...
    +  // We need to add theme_test for getting working test_basetheme and test_subtheme
    

    Remove 'We need'

  5. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    +  public function testFrontPage() {
    

    Rename to testNodesPage to make it in line with the $this->drupalGet('node');

  1. +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
    @@ -0,0 +1,17 @@
    +use Drupal\views\ViewExecutable;
    

    Should be on second line (move up)

  2. +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
    @@ -0,0 +1,17 @@
    + *
    + * @param ViewExecutable $view
    

    Delete

chris_hall_hu_cheng’s picture

@clemens.toolboom sadly the testbot occasionally just seems to freak out, has happened a couple of times already, I think as you have done, if the failure appears to be completely unrelated then queue for retest and hope for the best...

Berdir’s picture

  1. +++ b/core/modules/system/tests/themes/test_basetheme/test_basetheme.theme
    @@ -0,0 +1,17 @@
    +
    +function test_basetheme_views_pre_render(ViewExecutable $view) {
    +  $view->setTitle($view->getTitle() . ":" . __FUNCTION__);
    +}
    +
    +function test_basetheme_views_post_render(ViewExecutable $view) {
    +  $view->setTitle($view->getTitle() . ":" . __FUNCTION__);
    +}
    ...
    +
    +/**
    + * Implements hook_views_pre_render().
    + *
    + * @param ViewExecutable $view
    + */
    +
    +use Drupal\views\ViewExecutable;
    

    Looks like the docblocks are missing/incorrect.

    The file should start with a @file and then a short description of the whole file on the next line, have a look at other .theme files to see what they write.

    Then use statements.

    Then a Implements hook_something() per function, @param is not necessary.

    Also, you might have to either extend the title or do something different in the second function, or you can't assert both. Not sure if changing $view in post render even has an effect?

  2. +++ b/core/modules/system/tests/themes/test_basetheme/test_basetheme.theme
    --- /dev/null
    +++ b/core/modules/system/tests/themes/test_subtheme/css/sub-add.css
    
    +++ b/core/modules/system/tests/themes/test_subtheme/css/sub-add.css
    +++ b/core/modules/system/tests/themes/test_subtheme/css/sub-add.css
    @@ -0,0 +1,5 @@
    
    @@ -0,0 +1,5 @@
    +/* Thanks to http://jonathandean.com/2013/01/showing-the-current-breakpoint-name-when-testing-responsive-designs-using-only-css/ */
    +body:after {
    +  content: 'Showing test_subtheme';
    +  font-weight: bold;
    +}
    

    This shouldn't be necessary then anymore to detect that the theme is used.

  3. +++ b/core/modules/system/tests/themes/test_subtheme/css/sub-add.css
    index 0000000..b5bf1c1
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
    
    +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
    +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
    @@ -0,0 +1,17 @@
    

    Same for this file.

  4. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    +
    +  // We need to test for views content on the front page so need
    +  // We need to add theme_test for getting working test_basetheme and test_subtheme
    +  public static $modules = array('views', 'node', 'user', 'theme_test');
    

    Should be a /** docblock, with a @var array, see other tests.

  5. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    +
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Views theme integration test',
    

    I think getInfo() should have a @inheritdoc now too.

  6. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,102 @@
    +
    +    // Make base theme default first
    +    \Drupal::config('system.theme')
    ...
    +
    +    // Views kicks in
    

    Make sure that comments are complete sentences and end with a ".".

clemens.tolboom’s picture

Issue summary: View changes
Related issues: +#2167347: It's possible to enable a non-existing theme
FileSize
6.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,448 pass(es). View

#72

All done (doh)

#74

#1 Done
#1 regarding gettitle()

The title is a long trail of pre and post function names.

#2 Removed from patch

#3 Done

#4 DocBlock added

I cannot find consistent documentation. The best I could do was referring to ::setup

#5 Done

#6 Done

clemens.tolboom’s picture

I forgot to add the output form the test on both node/ pages

:test_basetheme_views_pre_render:test_basetheme_views_post_render

:test_basetheme_views_pre_render:test_subtheme_views_pre_render:test_basetheme_views_post_render:test_subtheme_views_post_render

So it all comes together.

clemens.tolboom’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +    // Views kicks in
    

    Must be a sentence ending with period.

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +    // Views kicks in
    

    Must be a sentence ending with a period.

:-(

clemens.tolboom’s picture

FileSize
6.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,938 pass(es). View
tsbah’s picture

Patch #34 works for me. D8 Bartik sub-theme. Thanks

clemens.tolboom’s picture

@tsbah it would help if patch #78 works for you too.

Can we RTBC this patch? This would help themers with the upcoming sprint weekend.

chris_hall_hu_cheng’s picture

It would help, a bunch of people, I can't RTBC as I contributed some of the code, essentially this is two line fix, that helps a lot of people (they are finding it and applying it. It shouldn't be this hard to get it committed should it?

When a need for test was stipulated clemens.tolboom dived in an quickly added that also (more work than the actual fix ;)). Would be great if it could be committed

Sadly the iterative nature etc. means that a bunch of people have already breezed through and RTBC and then we reset and have to start again.

Two line fix (which has never been contended), a bunch of people struggling with the issue, three months and over 80 comments later, still not committed.

clemens.tolboom’s picture

@chris_hall_hu_cheng thanks for your feedback.

IMHO you are fully entitled to RTBC if you take some time to check for all feedback from others like comments and code style are taken care of.

Your code for the fix is as it should be.

The tests were required so I stepped up to move this forward. And I got awesome feedback and a great learning experience. And I cannot RTBC this.

chris_hall_hu_cheng’s picture

@clemens.tolboom I going to be offline for a bit and just read your comment, but I agree, if no-one else reviews, by tomorrow evening then I will do as you suggest and review your test alongside the comment made.

The test is the main bit now anyway ;) thanks for stepping in on that my Drupal time is a bit limited as my current Employers dropped a big Expression Engine site (sshhh don't tell anyone :0) site in my lap that needed refactoring, learning that has kept me busy.

damiankloip’s picture

Nice work so far!

  1. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +    $type_values = array(
    ...
    +    $values['type'] = 'article';
    

    Can we just make both of these use the $var = array() syntax.

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +  public static $modules = array('views', 'node', 'user', 'theme_test');
    

    Ideally this should just use a test view, and not the frontpage view. All of these dependencies aren't necessary.

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +      'group' => 'Views theming',
    

    Can this just be 'Views' please.

clemens.tolboom’s picture

@damiankloip Thanks for the review.

  1. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +    $values['type'] = 'article';
    +    $values['title'] = $this->randomName();
    +    $values['promote'] = TRUE;
    +    $values['status'] = TRUE;
    +    $values['created'] = REQUEST_TIME;
    +
    

    Darn :-/ ... I missed that

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +  public static $modules = array('views', 'node', 'user', 'theme_test');
    

    That probably is best to make it independent.

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
    @@ -0,0 +1,107 @@
    +      'group' => 'Views theming',
    

    I like to keep this as we need way more tests for views theming I guess and having a seperate group helps people to find and write tests for theming.

I leave it to others to fixed these as I'm preparing for the sprint weekend. Maybe we take it on that day.

chris_hall_hu_cheng’s picture

Assigned: Unassigned » chris_hall_hu_cheng
Status: Needs review » Needs work

I am going to have a look at those last test tweaks.

I agree with @clemens.tolboom over the 'Views theming' group though, arguably I would have thought this issue could have been committed and a seperate issue raised for theme tests (after all this is essentially a bug in calling $module_handler->invoke() with the wrong type of parameter) in views but now we have taken that path, I would assume that there are other theme related tests on the cards for views? and views seems to be THE core module breaking it's tests out into the most groups.

Berdir’s picture

I would however not add a new test group for a single test. If anything, evaluate in a separate issue if there are other tests that are theming related, but there are problably not many theming-only tests in Views.

I guess the Views UI group is about the Views UI itself, not the UI that it creates :)

+1 to what @damiankloip said from me, let's keep this in Views until there are actually enough tests so that a separate group would make sense.

chris_hall_hu_cheng’s picture

Ok views it shall be :)

chris_hall_hu_cheng’s picture

FileSize
6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,897 pass(es). View

Adapted the test written by @clemens.tolboom based on the feedback in comment #84

Moved to views group and now uses one of the existing test views, reducing the number of module dependencies and the need to create a piece of content.

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review
clemens.tolboom’s picture

Status: Needs review » Needs work

Patch rework looks great :-)

@chris_hall_hu_cheng can you provide an interdiff?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
@@ -0,0 +1,93 @@
+	$this->enableViewsTestModule();

white space (tab)

chris_hall_hu_cheng’s picture

FileSize
3.46 KB

interdiff attached, apologies still a 'noob' around here.

Apologies for the tab also, for first time in ages I am doing non-drupal contract work and was rushing around trying to do this on a non-druapalized dev. environment :) during lunch yesterday. Was hoping it could get committed before anybody started trying sub-themes at the weekend sprints.

I can re-patch with any other comments/observations or changes this evening (assuming no big technical alterations).

Noe_’s picture

Status: Needs work » Reviewed & tested by the community

RTBC

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

The actual patch needs to be updated too.

There should be three files. A test-only patch that fails testing (only needed once but the tests changed quite a bit, so lets' add it again once), a full patch with fix and test (this will be committed) and if applicable, an interdiff that shows the difference to the previous patch (if applicable)

chris_hall_hu_cheng’s picture

OK, cheers, understood, I should be able to fit that in later today.

chris_hall_hu_cheng’s picture

Assigned: chris_hall_hu_cheng » Unassigned

Whoops forgot to say, I know there is lot of Drupal activity this weekend, if anybody else wants to grab this and finish it then go ahead I will pick it up again later today when I get back from my day trip if no-one has :)

Noe_’s picture

FileSize
4.96 KB
FAILED: [[SimpleTest]]: [MySQL] 63,128 pass(es), 4 fail(s), and 7 exception(s). View

@Berdir Did you mean something like this, with a failing patch?

Xano’s picture

@Noe_: yes. This allows others to verify the existence of the bug easily.

clemens.tolboom’s picture

Status: Needs work » Needs review
chris_hall_hu_cheng’s picture

Status: Needs review » Needs work

@Noe_ @Xano :

I believe that extraneous tab pointed out in #91 needs to be fixed, and new passing patch, failing patch and interdiff against 78 all need to be added in one comment, before this can be reviewed again.

If no one else does that I will pick it up this evening and do that, currently on a day trip so a bit devel opportunity challenged ;)

chris_hall_hu_cheng’s picture

Assigned: Unassigned » chris_hall_hu_cheng

I am back now so will sort the files out.

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 63,703 pass(es). View
4.96 KB
FAILED: [[SimpleTest]]: [MySQL] 63,203 pass(es), 2 fail(s), and 6 exception(s). View
3.47 KB

Hopefully I got that right.. these should be ready for review again now.

The last submitted patch, 97: core-views-2084907-97--fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 102: core-views-2084907-102-fail.patch, failed testing.

The last submitted patch, 102: core-views-2084907-102-fail.patch, failed testing.

chris_hall_hu_cheng’s picture

Status: Needs work » Needs review

Resetting to needs review as the fail patch was supposed to fail.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, yes, this looks good to me, has been confirmed to work, tests looks OK I think.

chris_hall_hu_cheng’s picture

If I have to re-roll this patch because it stops applying correctly can I automatically set it back to RTBC or does someone else have to come along and review it again?? What is standard practice?

Eventually core code will drift away from it.

Berdir’s picture

Depends, if you are confident that it's all good and it was an easy re-roll (e.g. just some context changes, no actual change in your code), then keeping it at/setting back to RTBC is ok. I usually compare the actual patch files to see if there are any relevant changes.

If you're not 100% sure, ask for another check.

chris_hall_hu_cheng’s picture

102: core-views-2084907-102.patch queued for re-testing.

Berdir’s picture

Title: Warning: Illegal offset type in isset or empty in Drupal\Core\Extension\ModuleHandler->loadInclude() » hook_views_pre_render() is broken if your theme implements a base theme

Closed #1972768: hook_views_pre_render is broken if your theme implements a base theme as a duplicate, that issue has however a better title, so stealing the title from there.

chris_hall_hu_cheng’s picture

@Berdir good spot on the duplicate issue. A couple of lessons here though, the original issue didn't contain the text of the error message (as it was written by someone who already knew where the problem was) If it had it may have been found when this issue was posted and or by people looking for a patch for their error later.

The second lesson is that once we found where the problem occurred, those of us involved should have searched the issue queue again.

Will bear both of those in mind in the future.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0a1e9aa and pushed to 8.x. Thanks! I credited the folks who worked on #1972768: hook_views_pre_render is broken if your theme implements a base theme as well.

+++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.theme
@@ -0,0 +1,24 @@
diff --git a/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php

diff --git a/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php b/core/modules/views/lib/Drupal/views/Tests/ViewsThemeIntegrationTest.php
new file mode 100755

Files should be created with file mode 644. Fixed during commit.

clemens.tolboom’s picture

YEAH :)

@alexpott how can we git diff these new file modes?

Jeff Burnz’s picture

Awesome, thank-you so much to everyone who fixed this! I am very grateful to all those who worked on fixing this, cheers!

Status: Fixed » Closed (fixed)

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