An empty hook_theme() causes fatal errors in theme.inc

Looks good to me if testbot comes back green.

Casting would be also possible ... here is a version which a test included.

  $result = $function($cache, $type, $theme, $path);
  foreach ($result as $hook => $info) {

This is the same as in Drupal 6 https://api.drupal.org/api/drupal/includes%21theme.inc/function/_theme_p... and Drupal 7 https://api.drupal.org/api/drupal/includes%21theme.inc/function/_theme_p... I am all for this new found babysitting but if it was fine for two releases, it'll be fine for a third. Why are you implementing hook_theme / how are you implementing if it does not look like return array(...) ?

It's not a normal hook, after all, but still: what are you doing that hook_theme is empty? I really, really would like to know.

hook_theme() is the only one because all that logic is properly fixed in ModuleHandler, ... this checks that the result "isset"

Just because it was wrong before doesn't make it right now. If we don't fix it this just falls into all the numerous "what's a hook?" inconsistencies we've been confusing people with for years. Also, its a trivial fix so why would it be a big deal?

Back to needs review.

A fatal error that doesn't give you a useful message about what you did wrong is a bug. Period. This should be fixed. (It should have been fixed long ago, but let's fix it now.)

Then let's at least NOT do this test. It's silly to add a whole new module needing enabling further slowing down poor bot. That's my problem.

Of course, there's no answer to the repeated question "what are you doing that it's not an array" instead we have theory ("it's a bug") again and again. I am sick of theories.

It's not that an empty hook_theme implementation or a hook_theme implementation that forgets a return statement at the end (same result) isn't a bug. It's that the current handling of that bug is utterly useless to help a developer fix it. "Fail fast, fail cheap, fail *usefully*. If you can fail gracefully, even better.

(Background: I accidentally wrote an empty hook_theme implementation in sample code for the training class Palantir is running this week without realizing it, and proceeded to confuse an entire class full of people with a totally unhelpful error for 10 minutes before we figured out WTF was going on. That's 9 minutes and 58 seconds longer than it should have taken. And allowing a null return to be treated as an empty array means that it would have been a non-issue, quickly resolved when the students began writing a hook_theme implementation 15 minutes after that.)

I have no opinion on whether or not we burn extra testbot cycles on it.

I totally agree that this simple bug should be fixed. Regarding test coverage ... we could also add a state check in the theme_test and return NULL when needed.

Sounds great! Let's do that.

.

+++ b/core/includes/theme.inc
@@ -463,7 +463,7 @@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
+    $result = (array) $function($cache, $type, $theme, $path);

Needs a comment as to why we're casting.

There we go.

rerolling

Rerolling and tested this and it seems to fix the issue. There was a nice fatal before the patch and after the patch it was fine.

I don't think it's necessary to fix this during RC.

This looks like it just needs a reroll and we're off to the races

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -6,6 +6,9 @@
+    return NULL;

Nit: this can just be return;

Revisited the approach, I think the cast + test coverage are fine, so just a re-roll seems plenty here.

Rerolled per comment #35 + #36

Rerolled patch against 9.3.x
Attached Rerolled diff against #37

Fixed Failed test #39
Added interdiff

Reroll the patch #39 with Drupal 9.5.x

Fixed test case in #44

As mentioned in #35 and #36, I just verified the re-rolled patch #46 in Drupal 9.5.x version. Patch applied successfully.

Also the return statement was updated as mentioned in #36

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -12,6 +12,9 @@
  * Implements hook_theme().
  */
 function theme_test_theme($existing, $type, $theme, $path) {
+  if (Drupal::state()->get('theme_test_theme_null')) {
+    return;
+  }

Thanks for the patch @smustgrave

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reroll the patch for the current version 10.1.x as latest patch was not compatible.

Sorry, forgot to add patch

Re-uploading the patch and rerolldiff

Fixed test failure.

Fixed corrupt patch and added it as MR.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Patch against 10.1.x

Interdiff #52-#57

@Rishabh Vishwakarma what was the base for your re-roll ?

Can you post interdiff ?

As you can see there is already a MR raised against 10.x branch there is no need for patches. You can just continue working on the MR. Thanks.

edit : Seems you added the details before this comment got published.

As you can see there is already a MR raised against 10.x branch there is no need for patches. You can just continue working on the MR. Thanks.

This point still stands. Your patch is identical to MR.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This could use an issue summary update. What is the proposed solution as this is touching a number of files? May need framework review too
Recommend using issue summary template.

Left a review, there's a lot of out of scope changes here.