Problem/Motivation

Currently, \Drupal\business_rules\Util\BusinessRulesProcessor stores information about every Event that has run in a page request in the database's key_value_expire table, with the collection business_rules.profile.

There are several issues with this approach:

  1. Critically, it doesn't qualify whose session the data belongs to, so two users who happen to make a page request that involves BusinessRulesProcessor at the same time will run into issues... actions might not run at all, or might run twice.
  2. It causes a lot of database traffic, which slows down the site (#3191811: Adding a dependent field with the Business Rules module makes the loading time and queries increase abnormally, #3168647: Business Rules Choking DB server with this query - key value pair)
  3. The teardown logic causes issues with tests (#3133687: Wrap keyvalue delete in BusinessRulesProcessor destruct) and Drupal 9 (#3187013: Error: Call to a member function prepare() on null after updating to Drupal 9.1.0 / PHP 7.4.11)

For context, note the current behavior was added in commit 1bcf53d by @yuriseki on Sat Apr 8 02:27:09 2017 -0300 (with the commit message "Revision 1"), i.e.: the third commit, i.e.: before the first release (8.x-1.0-alpha1).

Proposed resolution

Looking at where data is stored to business_rules.profile, it looks as if the data being loaded/stored is only valid for the request.

If this documentation is correct, then storing data in the key_value_expire table is the wrong place to store it. Sounds like drupal_static() would be a better choice, as that only sticks around for the request.

Remaining tasks

  1. Determine what functionality the business_rules.profile is intended to provide, and add tests
  2. Write a patch
  3. Review and feedback
  4. RTBC and feedback
  5. Commit
  6. Release

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#23 Schermata 2021-03-15 alle 18.54.57.png65.21 KBkris77
#17 3202612-17--test-only.patch9.49 KBmparker17
#17 interdiff--15-17.txt1.25 KBmparker17
#17 3202612-17--cache-business-rules-processor-to-memory.patch10.95 KBmparker17
#15 interdiff--12-14.txt467 bytesmparker17
#15 3202612-14--cache-business-rules-processor-to-memory.patch10.93 KBmparker17
#12 interdiff--11-12.txt11.74 KBmparker17
#12 3202612-12--test-only.patch9.51 KBmparker17
#12 3202612-12--cache-business-rules-processor-to-memory.patch10.98 KBmparker17
#11 interdiff--9-11.txt4.37 KBmparker17
#11 3202612-11--test-only.patch10.52 KBmparker17
#11 3202612-11--cache-business-rules-processor-to-memory.patch11.91 KBmparker17
#9 3202612-9--cache-business-rules-processor-to-memory.patch10.81 KBmparker17
#9 interdiff--7-9.txt609 bytesmparker17
#9 3202612-9--test-only.patch9.42 KBmparker17
#7 3202612-7--cache-business-rules-processor-to-memory.patch10.65 KBmparker17
#7 interdiff--5-7.txt8.32 KBmparker17
#7 3202612-7--test-only.patch9.25 KBmparker17
#5 3202612-5--cache-business-rules-processor-to-memory.patch2.2 KBmparker17
#4 3202612-4--cache-business-rules-processor-to-memory.patch1.7 KBmparker17

Comments

mparker17 created an issue. See original summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Related issues: +#3191811: Adding a dependent field with the Business Rules module makes the loading time and queries increase abnormally, +#3133687: Wrap keyvalue delete in BusinessRulesProcessor destruct, +#3187013: Error: Call to a member function prepare() on null after updating to Drupal 9.1.0 / PHP 7.4.11

(add related issues)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Related issues: +#3168647: Business Rules Choking DB server with this query - key value pair

Found #3168647: Business Rules Choking DB server with this query - key value pair as a related issue (thanks @colan!)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
StatusFileSize
new 3202612-4--cache-business-rules-processor-to-memory.patch1.7 KB

Here's an initial patch with no tests.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
StatusFileSize
new 3202612-5--cache-business-rules-processor-to-memory.patch2.2 KB

Now that a bunch of stuff has been merged to 2.x, I've re-rolled the patch in #4.

No changes - just a simple re-roll - so no interdiff necessary.

bmustafa’s picture

bmustafa commented

i confirm that #5 has fixed the problem #3187013 for me on latest release of alpha2 of business rules.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3202612-7--test-only.patch9.25 KB
new interdiff--5-7.txt8.32 KB
new 3202612-7--cache-business-rules-processor-to-memory.patch10.65 KB

Thanks to the fantastic test added by @Taran2L in #3120890: Configuration entities should provide dependencies (on other BR entities), I was able to write a test for the code paths in BusinessRulesProcessor which call $this->util->getKeyValueExpirable('process');.

Here is:

  1. a test-only patch to run the test on the old behavior (don't patch your site with this one)
  2. a patch with both the new behavior and the patch

Status: Needs review » Needs work

The last submitted patch, 7: 3202612-7--cache-business-rules-processor-to-memory.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review
StatusFileSize
new 3202612-9--test-only.patch9.42 KB
new interdiff--7-9.txt609 bytes
new 3202612-9--cache-business-rules-processor-to-memory.patch10.81 KB

Lets try that again with an explicit (dummy) assertion at the end of the test case.

Status: Needs review » Needs work

The last submitted patch, 9: 3202612-9--cache-business-rules-processor-to-memory.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
StatusFileSize
new 3202612-11--cache-business-rules-processor-to-memory.patch11.91 KB
new 3202612-11--test-only.patch10.52 KB
new interdiff--9-11.txt4.37 KB

Does this work?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review
StatusFileSize
new 3202612-12--cache-business-rules-processor-to-memory.patch10.98 KB
new 3202612-12--test-only.patch9.51 KB
new interdiff--11-12.txt11.74 KB

Apparently the PhpUnit syntax for mocking multiple calls to a method with the same name but different parameters and/or return values has changed.

The last submitted patch, 12: 3202612-12--cache-business-rules-processor-to-memory.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 12: 3202612-12--test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review
StatusFileSize
new 3202612-14--cache-business-rules-processor-to-memory.patch10.93 KB
new interdiff--12-14.txt467 bytes

Proving this worked before might be hard given I need the database... might have to switch my test from a Unit test to a Kernel test for the test-only patch.

Here's an attempt to fix the test fails in the patch anyway. (if it complains about drupal_static too, I might have to switch it to a Kernel test as well).

Status: Needs review » Needs work

The last submitted patch, 15: 3202612-14--cache-business-rules-processor-to-memory.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
StatusFileSize
new 3202612-17--cache-business-rules-processor-to-memory.patch10.95 KB
new interdiff--15-17.txt1.25 KB
new 3202612-17--test-only.patch9.49 KB

Lets try using Kernel tests instead.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review

Great! This is now ready for review.

If you're patching your site, use the 3202612-17--cache-business-rules-processor-to-memory.patch, as that contains the fix.

I think at this point, given that...

  1. the test in the test-only patch diverged from the test in the working patch a while ago;
  2. I've already spent more time on this issue than I was expecting;
  3. there seems to be an ongoing discrepancy between my local test environment and Testbot (all the tests since #7 have been passing locally only to fail on Testbot);
  4. adding test-only patches will make the issue more confusing for people just wanting to apply the patch and move on;
  5. the error on 3202612-17--test-only.patch is because the key_value_expire table is missing, and it is being created through some mechanism other than hook_schema() which will be non-trivial for me to figure out; and;
  6. I set out to cretae the test-only patch to help me fix tests for the real patch (whose tests are now passing)

... I'm going to give up on the test-only patches for now, unless the module maintainer(s) specifically want me to put effort into it before merging the patch.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Update the issue summary now that I know the UI, API, and Data Model changes.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@bmustafa, could I request you review and test the 3202612-17--cache-business-rules-processor-to-memory.patch patch in #17, and provide your feedback?

bmustafa’s picture

bmustafa commented

@mparker17,

i confirm patch #17 is perfect with me, and i have cleared my site cache, and the error does not appear, i am confirm all positive on my side.

$ composer update
Gathering patches for root package.
Removing package drupal/business_rules so that it can be re-installed and re-patched.
  - Removing drupal/business_rules (2.0.0-alpha2)
Deleting modules/contrib/business_rules - deleted
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 3 updates, 0 removals
  - Upgrading commerceguys/addressing (v1.1.1 => v1.2.0)
  - Upgrading commerceguys/intl (v1.0.7 => v1.0.8)
  - Upgrading psy/psysh (v0.10.6 => v0.10.7)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 3 updates, 0 removals
  - Downloading commerceguys/addressing (v1.2.0)
  - Downloading commerceguys/intl (v1.0.8)
  - Downloading psy/psysh (v0.10.7)
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Upgrading commerceguys/addressing (v1.1.1 => v1.2.0): Extracting archive
  - Installing drupal/business_rules (2.0.0-alpha2): Extracting archive
  - Upgrading commerceguys/intl (v1.0.7 => v1.0.8): Extracting archive
  - Upgrading psy/psysh (v0.10.6 => v0.10.7): Extracting archive
  - Applying patches for drupal/business_rules
    https://www.drupal.org/files/issues/2021-03-15/3202612-17--cache-business-rules-processor-to-memory.patch (d9_fix_business_rules1)

Package container-interop/container-interop is abandoned, you should avoid using it. Use psr/container instead.
Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.
Generating autoload files
51 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

vivo@DESKTOP-F22948O MINGW64 /d/wamp64/www/workbench/workbench_d9_test1
$
kris77’s picture

kris77 commented

In my content type I have 5 dependent fields to select the various car models.
So my selection is make, model, engine, etc ...(image attached.)

@mparker i have update from 8.x-1.0-beta13 to 2.0.0-alpha2, then apply your patch, but I haven't seen any improvements.

it always takes a long time.

kris77’s picture

kris77 commented
StatusFileSize
new Schermata 2021-03-15 alle 18.54.57.png65.21 KB

Image attached...

  • colan committed 833a1dd on 2.x authored by mparker17 
    Issue #3202612 by mparker17: Cached event data to memory instead of the...
colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Needs review » Fixed

Given the positive reports here and elsewhere, I'm merging this and cutting a new release.

#22: If you can prove that it's this module and not your set-up, please either reopen or create a new ticket.

Status: Fixed » Closed (fixed)

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