Hello jhodgdon!

I'm Advanced Help current maintainer for Drupal 8 module and I was thinking on come back to work on my port but found #2592487: [meta] Help system overhaul and I would prefer to help you to fix the hook_help instead.

So today I downloaded your module and found a bug which I just fixed.

I attach a patch and let me know if there is something that I can do to move forward this.

CommentFileSizeAuthor
#2 2873475-WSOD-help-topic-1.patch2.39 KBgnuget

Comments

gnuget created an issue. See original summary.

gnuget’s picture

gnuget
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new 2873475-WSOD-help-topic-1.patch2.39 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Hm. This patch doesn't seem quite right:

+++ b/src/HelpViewBuilder.php
@@ -43,14 +44,17 @@ class HelpViewBuilder extends EntityViewBuilder {
+    $this->themeRegistry = $theme_registry ?: \Drupal::service('theme.registry');

This should not be optional but required.

Also, can you give more details about the WSOD and why this patch fixes it? I guess I haven't tried running the tests on a recent 8.3.x or 8.4.x probably...

gnuget’s picture

gnuget
Primary language Spanish
commented

Sure!

Basically Drupal\config_help\HelpViewBuilder overwrite createInstance to inject $container->get('token') but forgot to inject: $container->get('language_manager') (see EntityViewBuilder::createInstance) and that throws the following error:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getRuntime() on null in Drupal\Core\Entity\EntityViewBuilder->getBuildDefaults() (line 175 of core/lib/Drupal/Core/Entity/EntityViewBuilder.php).

Also after to inject the dependency I got this warning:

Warning: Parameter 1 to Drupal\config_help\Element\TextSections::preRenderSections() expected to be a reference, value given in Drupal\Core\Render\Renderer->doRender() (line 376 of core/lib/Drupal/Core/Render/Renderer.php).

Which basically is because: \Drupal\config_help\Element\TextSections::preRenderSections() shouldn't expect a reference but a value (see Drupal\Core\Render\Renderer::doRender() in this part:

        $elements = call_user_func($callable, $elements);

There should exists an & before the $elements if it wanted to pass the reference instead to the value, in my patch I just removed the & from the function signature.

Hm. This patch doesn't seem quite right:

+++ b/src/HelpViewBuilder.php
@@ -43,14 +44,17 @@ class HelpViewBuilder extends EntityViewBuilder {
+ $this->themeRegistry = $theme_registry ?: \Drupal::service('theme.registry');

This should not be optional but required.

I'm agree with you but that is how it appears in the parent class (see Drupal\Core\Entity\EntityViewBuilder::__construct()) so I didn't want to change it.

  • jhodgdon committed 0b79468 on 8.x-1.x authored by gnuget 
    Issue #2873475 by gnuget: Fix WSOD and test failures when running in...
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Fixed

So, if I didn't say it before.. and I didn't -- THANKS for your interest in reviving this module and fixing it up!

So, I think when you said language manager in #4, you really meant the theme registry. It looks like that was added to the core EntityViewBuilder class sometime between 8.1.x and 8.2.x, which is why my class didn't have it -- I created this a while before that and apparently hadn't run the tests since 8.1.x. So, I adopted pretty much your patch, but not making the theme registry optional as you did. I don't see a reason for that. I think the parent class did this to maintain backwards compatibility, but we don't have to worry about that since this module is still a sandbox so it hasn't had a release yet.

And I agree you are correct about preRender functions not wanting &$element. So, adopted that part of your patch too.

Anyway, I've now run the tests for this module against the latest Drupal 8.3.x (I think Drupal 8.4.x is not much different). Your patch fixes all the failures. So, committed (with changes noted above). Thanks again!

Status: Fixed » Closed (fixed)

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