Cascadia
Account created on 8 November 2007, over 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

The latest MR is not complete; it contains some changes that are necessary to the solution, including a test case to prove the eventual solution, but it is not yet intended to solve the entire issue. Specifically, the ConditionManager is not yet changed in the MR, and that's the ultimate point of this whole issue.

I will set the status to "Needs review" when it is ready to review. I have now set the current status to "Needs work" to make this point more clear.

Thank you for the offer to help. You're the first one to offer - I wish I had that offer 5000 lines of code and 100 hours of work ago ...

I am currently prototyping what I think is the last of the needed changes in πŸ“Œ [10.3] Convert RulesAction plugins and discovery to attributes Active , since the RulesAction plugins affect only Rules and not core. I have committed some of the changes made there, and I have added similar changes that I know work into the above MR for this issue. When RulesActionManager is demonstrated to be able to discover RulesActions plugins either by Annotation or Attributes (or both) in that issue, then we will have a solution for this issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Current status:

RulesActions plugins were all re-written to use Attributes in addition to the (working) Annotations. The RulesActionManager was partially re-written to deal with:

1) Annotation-only discovery. This is overridden for Rules, and continues to work.
2) Attribute discovery. This almost works, not sure where the 4 new errors are coming from. See https://git.drupalcode.org/project/rules/-/jobs/2164620 for the failing tests.
3) Discovery of both Annotations and Attributes at the same time. This does not work yet. After 2) is fixed we can make more progress here, but as it stands now this is picking up the wrong ContextDefinition class - it uses the core ContextDefinition for some reason when it should be using the Rules version, and the core version does not have the Rules-added toArray() method. See https://git.drupalcode.org/project/rules/-/jobs/2164476 for the failing tests.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Yes, that's what I was looking for. This is the problem I thought.

I did a search and found it was never fixed in core Drupal 7. See πŸ› [D7] PhpMail : broken mail headers in PHP 8.0+ because of LF characters Needs work . There's a patch in that issue which should fix the problem for you

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Almost certainly this is due to bad message headers. Please attach a text file with your unformatted raw message headers.

#3261886: html tags not interpreted β†’
πŸ› PhpMail : broken mail headers in PHP 8.0+ because of LF characters Fixed

In short though, Drupal core changes line terminators in the headers to try to fix and avoid long-standing problems with PHP using the wrong line terminators, but in PHP 8.2 this got changed. I know we fixed this in Drupal core 9+ a long time ago but I don't know about D7...

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Ok, we're back to working.

I'm going to revert the changes to the ContextDefinition classes and put those changes into a separate issue.

ContextDefinition is too basic a function, which affects all Rules events, conditions, and actions. Significant changes to these classes shouldn't be buried in an issue about RulesActions, even though those changes are needed for this issue.

I will commit the ContextDefinition changes in a separate issue first, then come back to this ...

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Thanks for looking at this.

The latest test results on GitLabCI still show the same results: Everything works in D10.2, but the above errors happen in D10.3. You can also see the same behavior in the MR test output above. I re-ran both just now (and I had run them a dozen times over a few days before opening this issue just to be sure it wasn't a temporary failure with GitLabCI.)

https://git.drupalcode.org/project/honeypot/-/pipelines/223153
https://git.drupalcode.org/project/honeypot/-/pipelines/218495

In general, it doesn't matter if it works locally. If it doesn't run on GitLabCI using the controlled environment, then there's a problem.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

I meant to post to let you know I marked it supported again, but I forgot. Glad you noticed that.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

The one test failure is intentional at this point - CoreUserRoleConditionTest was added to demonstrate the failure described above, namely that the Rules ConditionManager doesn't find core Conditions in D10.3 because the core Conditions dropped their annotations between D10.2. and D10.3. When the Rules ConditionManager fixes are added to the MR, then this test will pass - that will help prove the fix and prevent / detect similar breakage in the future.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

@sl27257: This is a support issue. "Fixed" means the question has been answered. It was answered completely. This issue is "fixed".

So when can we expect to see a solution?

If you want to hire me and pay me, then you will get to dictate when and how I work on this issue.

Until then, *I* get to decide how I spend my free time. I am not employed to work on Drupal in any way. NO ONE is employed to work on the Rules module in any way. If it is important to *you* that a fix happens promptly then perhaps YOU can spend dozens of unpaid free hours contributing back to the community. OR perhaps you can fund the development done by volunteers like myself. If you don't care enough to put your own time or money into the fix, then please shut up.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Question: on the MR, there is an option "Delete source branch when merge request is accepted."

I want to make sure I'm correct in understanding that source branch = MR/feature branch, and target branch = the 7.x-2.x branch... right?

Am I correct that checking it means that once the MR is merged, the feature branch I created for this MR will be deleted?

Yes, that's what it means. But you shouldn't usually have to worry about that - that box is not checked, by default, and it's OK to leave it like that.

I'm not sure why the Drupal administrators set it up that way, and what they have in mind for cleaning up all the old feature branches accumulating on gitlab, but for now I am just going with the default settings.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

This dependency is deliberate.

Rules 3 will not work with core Drupal 10.3, so do not upgrade your version of core yet if you need Rules - and in fact the Typed Data dependency should prevent you from upgrading core and should prevent you from having problems. This is being worked on and will be fixed in the near future. See πŸ“Œ [10.3] Update ConditionManager to support PHP Attributes Active .

Core Drupal 10.3 introduced a change that broke Rules 3. Fixing that would require major changes, and those changes would break compatibility with Drupal 10.2 and Drupal 9.

Rules 4 was created so that we can make these changes to support Drupal 10.3 and Drupal 11 without breaking support for earlier versions of core.

So Rules 3 is for Drupal 10.2 and below, Rules 4 is for Drupal 10.3 and above. Rules 4 uses Typed Data 2, while Rules 3 uses Typed Data 1.

Rules 4 is not ready to use on core Drupal 10.3 yet - that is why there is no release.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Now fixed in release 7.x-1.28. Or you can stay on 7.x-1.26.

As noted in the release notes, there are still no functional differences since 7.x-1.26 - this new release was simply to configure this module for the new testing system and to eliminate some coding standards notices in the tests.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Yes, sorry about that. I will revert the change.

As a side note, with the switch to GitLabCI we no longer have the ability to test against PHP 5, which was something I always insisted on because I didn't want to break existing sites. But without that check in place some things like this are going to slip through from time to time.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Actually, I said D7, but this is an issue for the 8.x-1.x branch so I don't know what I was thinking ... maybe just seeing the t() function triggered me ...

Regardless, I think the argument still holds, although it is less strong in the case of the 8.x-1.x branch. We probably shouldn't be changing translatable strings in patch releases.

πŸ“Œ | Honeypot | phpcs issue
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ“Œ | Honeypot | phpcs issue
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

No further information provided. If this is of interest to anyone, feel free to re-open the issue, answer my questions from #3, and if possible provide a patch.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Fixed by the commit in πŸ“Œ [meta] Add scalar type hinting and return value typing Fixed . The solution was as in #5 - use parameter and return type hints to enforce values being passed to the placeholder resolver.

The related issue in Rules will be fixed in the Rules issue queue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

The root problem is that there are cases when a data processor plugin is being called on to process a NULL value.

Yes, patches like #12 can narrowly avoid the problem by checking on the value inside the data processor, but as data processors are plugins that means that each and every data processor needs to know it has to do this, then needs to do it. That is not a scalable solution, and it is error prone.

I better solution IMO is outlined in my comment at #3095216-4: [meta] Data Processors β†’ .

Specifically, DataProcessor plugins should have mandatory method defined in the plugin interface to declare the data type(s) they can process. That way, we can see if the DataProcessor applies to our data before we try to process our data. That's far better than just checking for NULL, because as described in the related issue #2849810: Data processors do not check type before performing type-specific operations β†’ the problem isn't always a NULL value - however it IS always a value of the wrong data type.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

This will also resolve the related issue I just linked.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Well I violated my own request of not posting patches in a meta ... I opened a MR because the remaining missing parameter and return type hints are scattered all over the code - not in one compact place. I figure one MR that we can add to and have tested is now the easiest way to resolve the remaining bits in a quick and efficient manner.

There are probably some places we could use nullable type hints, but in general I'm not a fan of passing around null then having to costantly check if the value is null or not, so my preference is usually to enforce a type with a value and don't accept or return null unless we're interacting with core functions that give us no choice.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Tests run now, except for ActionFormTest.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Had to remove the declare from ActionFormTest and add a @todo, but now all the Kernel, Functional, and FunctionalJavascript tests work with the declare.

Adding the Unit tests to the MR now.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Hid patches #38 and #42 to reduce confusion. Those patches are for core, not for the Rules module. And as I said in #40, that will only help if user_roles is the only core Condition you are using. In principle, adding annotations back to *all* the core Conditions (or simply leaving them there in the 10.2 to 10.3 transition) will avoid this problem. But it's not a long-term solution.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

@hargobind: Can you convert your patch into a MR? Drupal.org does not run testing on patches any more - all contributions are expected to be through merge releases.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

I'm not sure why these need to specifically go *first*. If they didn't, then the $additional_annotation_namespaces parameter could be used in the plugin manager's __construct(), and that **DOES** have BC handling.

Also, why does Rules need to put its annotation classes into silly places?

Well, short answer is ask @fago. Long answer is that the whole Rules plug-in architecture was co-developed along with the Core plug-in architecture back in the pre-D8.0 days. And just like Core has remnants of silly things dating back to the D4.5 days, Rules still has remnants of code that was considered to be good practice when it was first written. While I have refactored large parts of Rules over the years, some of this stuff which was decided and implemented long before I became a maintainer has persisted because it works and has lots of tests to make sure it stays working.

Regardless, Rules Conditions are a fundamental part of Rules, and they have an extensive set of tests that have been running against the current and upcoming versions of Drupal core, on a weekly and per-commit basis, for something like 8 years. That's why this core change between minor point releases of core, which didn't raise any core deprecation notices or warnings, is so disruptive.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

There is no "quick fix" for a breaking core change between minor point releases of core (10.2 and 10.3) that affects pretty much every aspect of Rules. Everything works in 10.2 still. And BTW all the Rules extensive tests run properly in D10.3 and D11, with no deprecation warnings shown from the 10.3 change that broke everything. Fixing this requires a new major branch release of Rules, with all the BC implications of that which I have to manage.

The situation is still as I said in #17 and #23, except that since then I have made hundreds of commits and changed thousands of lines of code in a half dozen modules, with more work still needed. Maybe this week, if all goes well.

You could always contribute to the solution in some way, if this is important to you.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Not bad. I'd like to get these few issues fixed before moving on to the Unit tests.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

First commit is everything but the Unit tests, to see if any problems occur. I expect the Unit tests will be more of a problem.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
✨ | Rules | Add Gitlab CI
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

I turned the patch into an MR for Rules 4.0.x.
The status is still as described in #22 - this still needs an update hook.
Also, should probably figure out how to add deprecation notices for this in 8.x-3.x, for BC purposes, to help any contributed modules that might be using these events.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

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.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

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.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

This is just plain weird. When I copy the core FileFieldWidgetTest into the Honeypot namespace, with no changes (other than the namespace of course), it FAILS in core 10.3 and PASSES in core 10.2.

That just should not be - the core tests should always be working.

The test failure looks like this:

    Drupal\Tests\honeypot\FunctionalJavascript\FileFieldWidgetTest::testMultiValuedWidget
    WebDriver\Exception\CurlExec: Curl error thrown for http POST to
    http://localhost:9515/session/181e9c672d5c7f455268ba88c9d7bb3f/element/f.6D1A3CEA6617A6A3D67277607608CB4F.d.043A063B0C99C792D2A1EA30A95FB313.e.13/click
    
    The requested URL returned error: 400 Bad Request
    
    /builds/project/honeypot/vendor/lullabot/php-webdriver/lib/WebDriver/Exception.php:198
    /builds/project/honeypot/web/core/tests/Drupal/FunctionalJavascriptTests/WebDriverCurlService.php:152
    /builds/project/honeypot/vendor/lullabot/php-webdriver/lib/WebDriver/AbstractWebDriver.php:170
    /builds/project/honeypot/vendor/lullabot/php-webdriver/lib/WebDriver/AbstractWebDriver.php:290
    /builds/project/honeypot/vendor/lullabot/php-webdriver/lib/WebDriver/Container.php:231
    /builds/project/honeypot/vendor/lullabot/mink-selenium2-driver/src/Selenium2Driver.php:847
    /builds/project/honeypot/vendor/lullabot/mink-selenium2-driver/src/Selenium2Driver.php:841
    /builds/project/honeypot/web/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:122
    /builds/project/honeypot/web/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:202
    /builds/project/honeypot/web/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:133
    /builds/project/honeypot/vendor/behat/mink/src/Element/NodeElement.php:176
    /builds/project/honeypot/vendor/behat/mink/src/Element/NodeElement.php:186
    /builds/project/honeypot/web/core/tests/Drupal/Tests/UiHelperTrait.php:101
    /builds/project/honeypot/web/core/tests/Drupal/Tests/UiHelperTrait.php:161
    /builds/project/honeypot/tests/src/FunctionalJavascript/FileFieldWidgetTest.php:58
    /builds/project/honeypot/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Line 58 of FileFieldWidgetTest is $this->drupalLogin($this->adminUser);

The artifacts are no use in debugging this.

Not sure what to do next ...

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

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.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

"Needs tests" means automated tests, not manual testing.

πŸ“Œ | Honeypot | phpcs issues
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ“Œ | Honeypot | phpcs issues
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ“Œ | Honeypot | cspell issues
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia
πŸ“Œ | Honeypot | cspell issues
πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

2

x: The definition for the 'rules.reaction.test_rule.expression.conditions.conditions' sequence declares the type of its items in a way that is deprecated in drupal:8.0.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/2442603
2x in EventIntegrationTest::testHoneypotRejectEvent from Drupal\Tests\honeypot\Kernel

2x: The definition for the 'rules.reaction.test_rule.expression.actions.actions' sequence declares the type of its items in a way that is deprecated in drupal:8.0.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/2442603
2x in EventIntegrationTest::testHoneypotRejectEvent from Drupal\Tests\honeypot\Kernel

These two are in the Rules module, not Honeypot, and were taken care of by the Rules issue πŸ“Œ [11] Schema Validation errors: Incompatible types Fixed

1x: The Drupal\Tests\field\Traits\EntityReferenceTestTrait is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Instead, use \Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait. See https://www.drupal.org/node/3401941
1x in KernelTestSuite::suite from Drupal\Tests\TestSuites

I don't know what this is. Honeypot doesn't use EntityReferenceTestTrait.

1x: Installing the table sequences with the method KernelTestBase::installSchema() is deprecated in drupal:10.2.0 and is removed from drupal:12.0.0. See https://www.drupal.org/node/3349345
1x in EventIntegrationTest::testHoneypotRejectEvent from Drupal\Tests\honeypot\Kernel

Similar to all deprecation notices, this one is telling us that this was changed in core 10.2. That means if Honeypot is going to support versions of core below 10.2, it CAN'T make this change yet. Or at least, if it does make the change it has to insert conditional code into Honeypot to do the equivalent of "if core version < 10.3 then do something, else do something different". I don't plan to do that - instead, because of all the breaking changes in 10.2, 10.3, and 11, I plan to make a new branch of Honeypot which supports only core 10.3 and above. This deprecation will be fixed in the new branch, but not in the old since the old branch will never support core versions greater than 10.2.

As a side note, running tests on your own is not a good way to find and fix problems - you are reporting errors that depend on your testing setup (unknown to me) and the modules you have installed (unknown to me). As I said, Honeypot has automated tests which run on GitLabCI where the test environment and conditions are defined and known. If an error shows up in the automated tests, then we are aware of it! If you would like to provide a fix for any testing error that shows up, please open a new issue and link to the automated test output. But there is no need to open an issue unless you plan to create a patch.

Postponing this because the one deprecation about installSchema() does need to be taken care of eventually, but it will be in a new branch which does not exist yet.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

For future reference, Rules was using the old standard for sequence configuration schemas. This was something that was changed early on during the Drupal 8 core development development process, and I guess the Rules developers at the time missed this. I still find it surprising that this hasn't been noticed before! Simplified definition format for sequences in configuration schemas β†’

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Please read my comment in ✨ Add support for TFA Postponed: needs info - the same reasoning applies here. It is simply not a workable solution to have Honeypot know about and make exceptions for all the other contributed non-core modules that may need some forms excluded. Honeypot allows other modules and site builders to explicitly exclude forms, and that's where this should be done.

Also see ✨ Allow for more flexible form configuration Needs review for a proposal to let site builders do this through the UI.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

TR β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

This module is not very maintainable without tests. Even simple test cases would help.

Looking for contributions ...

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

I created a MR from the patch so that we can do automated testing on it.

Production build 0.69.0 2024