Problem/Motivation

I just figured out that on our Drupal setup, that uses symlinks, _rules_discover_module() triggers lot's of db queries in drupal_get_filename().
This is because _rules_discover_module() doesn't handle symlinks atm. and RulesAbstractPlugin::getIncludeFiles() doesn't check if a sane value is given as module parameter.
This can lead to calling module_invoke() with FALSE as module parameter. Which then again triggers drupal_get_filename() with non-sense.
I found the whole thing because of this: #2380361: Latest tweaks to bootstrap.inc can make Drupal sluggish when there is code that tries to load a file that doesn't exist

Proposed resolution

Add symlink support to _rules_discover_module().
Just call module_invoke() in RulesAbstractPlugin::getIncludeFiles() if there's a sane value for module.

Remaining tasks

Reviews needed.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#4 rules-symlink_support_for-2483729-4-D7.patch3.05 KBhargobind
#2 2483729-2-add-symlink-support-for-rules-discover-module.patch2.89 KBtr
rules-add-symlink-support-for-_rules_discover_module.patch2.89 KBdas-peter

Issue fork rules-2483729

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

Status: Needs review » Needs work

The last submitted patch, rules-add-symlink-support-for-_rules_discover_module.patch, failed testing. View results

tr’s picture

tr commented
Status: Needs work » Needs review
StatusFileSize
new 2483729-2-add-symlink-support-for-rules-discover-module.patch2.89 KB

Re-rolled against current HEAD.

chase.’s picture

chase. commented

I've always used a symlinked sites/all directory, but never noticed this issue until PHP 8.1: With FALSE being returned by _rules_discover_module(), PHP 8.1 complains with a deprecation warning, which even triggered Issue 1285856.

Long story short: Patch from #2 fixes this. Took a closer look at the resulting variable values in _rules_discover_module() and have no complaints ;-)

hargobind’s picture

hargobind
he/him
Location Austin, Texas
commented
StatusFileSize
new rules-symlink_support_for-2483729-4-D7.patch3.05 KB

The patch in #2 no longer applies against 7.x-2.x-dev. Attaching an updated patch.

I'm still testing this myself to see if it fixes the problem. I'll report back if I can.

jvogt’s picture

jvogt commented

Patch #4 applies cleanly to rules 7.x-2.13 with php 8.1 and core 7.95. It resolves the issue of the error message ("Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in drupal_get_path() (line 2974 of /[...]/includes/common.inc).") I haven't tested it beyond that.

robertinop’s picture

robertinop commented

Patch #4 also applies cleanly to rules 7.x-2.14, with PHP 8.1 and core 7.98. The message (similar to #5) is no longer logged:

"Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in drupal_get_path() (line 2970 of /[...]/includes/common.inc)."

tr’s picture

tr commented

There are still no reviews that say anything about testing this new feature and whether it works or not.

The "Deprecated function" warning is something from PHP 8, and is a separate issue from symlinks, and has no bearing on whether this patch achieves its intended goals.

The best way to prove this works, absent community participation in testing, is to write a test case using a symlink. The test case should fail in the current version of Rules because of the problem mentioned in the original post. Then after we apply the patch the test case should now work, which proves that the patch fixes the problem.

A test case is required for new features in Drupal, and Rules is using those same criteria when adding new features. I don't plan to commit this patch without feedback from the community about whether it works, but I could skip that part if there was a good and effective test case included with the patch.

tr’s picture

tr commented

This symlink issue is potentially causing all tests to fail under PHP 8 on GitLabCI (the test environment uses symlinks ...), so I've created an MR out of the patch in #4 to see if it solves those errors.

TR opened merge request !50

TR credited fjgarlin.

tr’s picture

tr commented

Restored MR to be identical to the patch + removal of the PHP 7.4 requirement from .gitlab-ci.yml.

Crediting @fjgarlin because he was the one who figured out that the testing problem might be because GitLab CI was using symlinks. See https://drupal.slack.com/archives/CGKLP028K/p1719869263598159

  • TR committed 50147af7 on 7.x-2.x 
    Issue #2483729 by TR, das-peter, hargobind, fjgarlin: Symlink support...
tr’s picture

tr commented
Status: Needs review » Fixed

I'm still not thrilled about committing this without tests, because it will affect about 100,000 legacy sites still running on D7.

On the other hand, I can't support anything about this module without the tests running properly on GitLabCI, so I don't really have a choice. I guess GitLabCI will have to be the practical test that symlink support is working.

Merged. Thanks to all who helped.

Status: Fixed » Closed (fixed)

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