Problem/Motivation

Creating/editing a field and clicking save appears to do nothing. The log contains a php error.

Steps to reproduce

1. Upgrade core to 11.x-dev, up to commit 7e27c7f or later.
2. Create a webform. Add a textfield.
3. Just give it a name and click save.
4. Nothing happens and the log has an error - Error: Call to undefined function Drupal\webform\Hook\_webform_token_render_more() in Drupal\webform\Hook\WebformTokensHooks->tokenInfo() (line 205 of ...\web\modules\contrib\webform\src\Hook\WebformTokensHooks.php).

Proposed resolution

If I add require_once __DIR__ . '/webform.tokens.inc'; near the top of webform.module, it fixes it, but it doesn't feel like the right solution. If somebody understands better what's going on in #3494908: Set skip procedural scanning for all modules in core maybe they have a better idea of what to do.

My guess is that before that commit, it somehow magically loaded webform.tokens.inc, but since that commit maybe changes how scanning works it now needs to be told about it, probably in the hook implementation somehow?

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#26 wfc_test_matrix_v2.png463.94 KBkaring

Issue fork webform-3539377

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

demeritcowboy created an issue. See original summary.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Issue summary: View changes

I cannot find commit 7e27c7f in the repo. Please check the commit hash.

demeritcowboy’s picture

demeritcowboy commented

https://github.com/drupal/core/commit/7e27c7fdd465c62004d2355bcc6a38f52d...

cilefen’s picture

cilefen commented

Because of the subtree split, Github commit hashes are different than Gitlab commit hashes.

karing’s picture

karing
Location 🇨🇦
commented

Bisected it down to:

`COMPOSER_MEMORY_LIMIT=-1 composer require drupal/core:11.x-dev#7e27c7fdd465c62004d2355bcc6a38f52d91b176 drupal/core-dev-pinned:${{ matrix.drupal }}`

https://github.com/colemanw/webform_civicrm/compare/KarinG-D11-2-2?expand=1

cilefen’s picture

cilefen commented
Priority: Normal » Critical

#3494908: Set skip procedural scanning for all modules in core and this change record indicate that adding `skip_procedural_hook_scan: false` to the services file is the correct way. Can someone test that and get a merge request started?

demeritcowboy’s picture

demeritcowboy commented

Thanks I tried it but it makes no difference. Cleared cache too.

Specifically this change

index 97fce5da1..a234ca1ee 100644
--- a/webform.services.yml
+++ b/webform.services.yml
@@ -1,3 +1,6 @@
+parameters:
+  webform.skip_procedural_hook_scan: false
+
 services:

   # Plugins.

I get the impression that that parameter is false by default. I'm thinking because the hook HAS been converted to new style, there is some other way you are supposed to tell it about the resources it needs loaded.

Or would a simpler fix be to just move the "render_more" function somewhere else that is always loaded? Or, since it's only used in two files, move it directly into the hook file?

nicxvan’s picture

nicxvan commented

Thanks!

This is because we set system to skip, but system has system_hook_info which needs to still execute.

#3540033: Do not skip scanning for procedural hooks in the system module

demeritcowboy’s picture

demeritcowboy commented

Thanks. I can confirm that core patch fixes it locally and also in the tests mentioned in comment 5.

cilefen’s picture

cilefen commented
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Possible duplicate

Ok, I am marking this as a pending duplicate but I am leaving it open so we don't forget it.

nicxvan’s picture

nicxvan commented

Yeah this issue is 100% core.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Category: Bug report » Task
Priority: Critical » Normal
Status: Postponed (maintainer needs more info) » Active
Issue tags: -Possible duplicate

While I agree that this is a bug in core, webform is still unnecessarily relying on automatic .inc file loading for helper functions called by that hook.

This is deprecated for removal in D12, while legacy hooks are supported until D13 (and not even formally deprecated yet), so you likely want to keep them around longer to support D10.

That means eventually, all functions in tokens.inc must move to .module and there is no reason to not do it now. that would fix this particular error but the token system is going to be extremely broken due to all non-converted hooks that are still in tokens.inc, such as the ones in token module.

So, changing this to a normal task to move that.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

In that case, perhaps the existing proposed resolution is the right solution. That is a smaller change than moving all the functions to the module file.

nicxvan’s picture

nicxvan commented

I really would recommend moving the functions, it's the expected resolution we planned for in core.

jrockowitz’s picture

jrockowitz commented
Priority: Normal » Critical

This feels like a release blocker. I am tempted to suggest we look for a simple temporary workaround or fix.

nicxvan’s picture

nicxvan commented

The functions in the tokens.inc files just need to move to .module files

To be safe we should also move views.inc functions

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Can we not add a require statement?

nicxvan opened merge request !678

nicxvan’s picture

nicxvan commented

Those includes are deprecated in 11.2 anyway so moving them is the right fix.

nicxvan’s picture

nicxvan commented

Failure was:

Webform Element Address (Drupal\Tests\webform\Functional\Element\WebformElementAddress)
     ✘ Address
       ┐
       ├ UnexpectedValueException: RecursiveDirectoryIterator::__construct(/builds/issue/webform-3539377/web/sites/simpletest/91902458/files/php/twig/68a5fc15c268d_page-title.html.twig_KxozsPVNro9Qk3xuL_VXDnfQA): Failed to open directory: No such file or directory

I am rerunning, I'm not sure that is related.

jrockowitz’s picture

jrockowitz commented

#20 the failure is unrelated and it is a random failure.

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

Yep, it passed with a rerun!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> This feels like a release blocker. I am tempted to suggest we look for a simple temporary workaround or fix.

It is a release blocker, yes. For Drupal core. This only happens in the core development version that will become 11.3, and the token system is currently very broken there. All token.module tokens simply do not exist.

Tests pass because they dont run against next minor.

It will and must be fixed in core. The core issue is RTBC and critical.

This is a useful change but it's just a D12 preparation task, nothing more.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
karing’s picture

karing
Location 🇨🇦
commented

Confirming that all is well with Drupal 11.2.* and Webform 6.3.x-dev - we only caught this because one of our iterations in the webform civicrm test matrix is with Drupal 11.x-dev, Webform 6.3.x-dev and CiviCRM dev-master.

wfc test matrix

karing’s picture

karing
Location 🇨🇦
commented
StatusFileSize
new wfc_test_matrix_v2.png463.94 KB
jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review

I am feeling that we need an MVP solution for webform.tokens.inc, and this might be the most straightforward approach.

nicxvan’s picture

nicxvan commented

Is there a reason not to just move them to the .module file?

That's the recommended approach for files relying on hook_hook_info

Edit if you want to do it the way you are you'll need those for each submodule using it.

The core issue has been resolved too.

jrockowitz’s picture

jrockowitz commented

The webform module's hooks are organized into includes, so it makes sense that an include/webform.tokens.inc would be added here.

https://git.drupalcode.org/project/webform/-/blob/6.3.x/webform.module?r...

jrockowitz’s picture

jrockowitz commented

I think the MR via #3546389: Error: Call to undefined function Drupal\webform\Hook\_webform_token_get_submission_value() completes the move from token functions to OOP hooks. I moved the webform.token.inc to includes/* because most of the legacy functional hooks are also includes/*.

When #3546389: Error: Call to undefined function Drupal\webform\Hook\_webform_token_get_submission_value() is fixed, I will also mark this as fixed to ensure everyone gets commit credit.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Priority: Critical » Normal

Since core has been fixed for ~3 weeks (see #3540033: Do not skip scanning for procedural hooks in the system module). Adjusting the title and criticality of this issue, since as Berdir mentiomed, this is really a task to prepare for D12.

jrockowitz’s picture

jrockowitz commented
Status: Needs review » Needs work