🇸🇰Slovakia @poker10

@seanr Can you open a new issue for this? We are also getting similar errors from bots even on 2.0.2.

Google reCAPTCHA v3 validation failed: The response parameter is invalid or malformed. Expected action did not match.

Google reCAPTCHA v3 validation failed: The response parameter is invalid or malformed. Expected hostname did not match. Expected action did not match.

Thanks everyone for working on this issue!

No need to be personal. Last comments seem to be a bit on the edge, so please focus on finishing the issue, so that we all can benefit from it. Thanks!

In January 2022, there was a German Court decision that including Google Fonts via Google URLs violates GDPR, because it sends IP address to Google. See for example: https://cookie-script.com/blog/google-fonts-and-gdpr

This is similar as other scripts / CDN resources.

greggles → credited poker10 → .

poker10 → credited poker10 → .

poker10 → credited poker10 → .

poker10 → credited poker10 → .

greggles → credited poker10 → .

This "override" code was added in #742246: Handle uncaught exceptions → in 2010, so no, this is not new and it is a part of Drupal for 14 years. The same code is also in D7 - with the difference, that there is explicitely check for updating, so this probably indicates we should be careful what the change in MR could affect:

https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/errors.inc...

$updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');

Drupal 8+ code was changed here to override also when installing: #2176105: Installer catches exceptions and manually re-prints them; does not use error/exception handler →

+  if (defined('MAINTENANCE_MODE')) {
+    return TRUE;
+  }
-  $updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');

I am curious if we can check some d.o. statistics about "Maintenance status" and "Development status" in comparision with gitlab commits. It can be interesting to see how many modules are Actively maintained and/or Under active development and have no commits for a longer period (or similar).

I often see that these two values does not reflect the reality in many modules (would say that maintainers set them mostly when creating a module and then never update it). Certainly there are exceptions and lot of modules has this set correctly, but the question is if it does not get too much space, so that the module maintainers will be abusing this to get the "icon".

Adding a related issue which propose to remove MAINTENANCE_MODE == 'update' (which is used in authorize.php as well).

@cmlara Not speaking directly about this issue, but there are multiple policies regarding Drupal Security Team and specific vulnerabilities - for example ones mentioned by this PSA: https://www.drupal.org/psa-2023-07-12 → , or other exceptions here: https://www.drupal.org/drupal-security-team/security-advisory-process-an... →

If you are concerned about specific policy, feel free to open an issue to discuss it. Thanks!

Re #5: OK, but I do not see a section about committers in that patch (if we do not consider all Initiative Leads and Leadership Team as committers).

Is there any progress with the Drupal CMS governance (since the leadership team has been announced)? Or what are the next steps/plans? Thanks!

I think it would be great to include also a list of committers in that file. IIUC, there seems to be only one committer currently - phenaproxima (as per the Dries presentation https://youtu.be/ELApwaOoyDM?t=3230).

We skipped the 2024-08-22 meeting. I am repurposing this issue for the next meeting.

+1 to RTBC. It would be great if we can get the button on both places (top + bottom), but if not, even the change for the link on the bottom will make a difference.

greggles → credited poker10 → .

greggles → credited poker10 → .

greggles → credited poker10 → .

A different logo was committed a month ago by the maintainer, see: 📌 Project Browser: Create a logo for Geofield module Needs review . I think we should close this.

Thanks for reporting and working on this. If $params['text'] will be a zero (a number), this code will behave differently on older PHP versions (the result will be TRUE) and newer PHP versions (the result will be FALSE):

if ($params['text'] == "") {
  ...
}

Also if it is possible, that $params['text'] can be NULL, then the patch will change the current behavior.

Thanks for reporting this and providing a patch. I cannot reproduce the issue. There is no issue summary and steps to reproduce are missing too.

Citing the StackOverflow issue:

Always a bad idea to use the @ operator. If something is broken, you should deal with it.

I am closing the issue. Feel free to reopen if this is still an issue - in such case, please update the issue summary with all required information. Thanks!

If there are no other options for highlighting the link, it would be great if we could do at least what was proposed by @fjgarlin in #4. Thanks!

Yes, that is what I meant (sorry if I was not clear enough). The clear cache behavior with an array is still being tested on lines which are left in that function. I think this change looks good now. Thanks!

This looks good to me, thanks!

Just to add, this comment was committed in #668932: Duplicate sanitzing of HTTP_HOST? → and it seems like there was a typo. The issue summary correctly mentioned drupal_environment_initialize(), but in the patch drupal_settings_initialize() was added instead.

Thanks for reporting and working on this.

I did some search and found out that the changes in test were committed in #333171: Optimize the field cache → . If we read the discussion in that issue, we can see that it was proposed to add the new variable cache_clear_threshold and it was in patch in #18. But then it was removed, but was left in the test (seems like unintentionally). So I agree we can remove this safely.

I think there is one question left here - do we need the rest of the test, or can we delete all other line from cache_clear_threshold to the end of the function? I do not see any difference between:

    // Clear two entries using an array.
    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value),
                       'Two cache entries removed after clearing with an array.');

and

    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2', 'test_cid_clear3'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear3', $this->default_value),
                       'All cache entries removed when the array exceeded the cache clear threshold.');

Just that the first is deleting two entries and second three entries. If I have not missed anything, I think we can safely remove the remaining part of the test in this cleanup as well. What do you think?

hestenet → credited poker10 → .

hestenet → credited poker10 → .

hestenet → credited poker10 → .

smustgrave → credited poker10 → .

greggles → credited poker10 → .

Thanks for working on this! It seems like there is still some work needed.

Issue summary seems to be outdated, as we are not fixing relative paths, but @imports.
Parent issue updated the tests, here the test changes are missing.

I am also a bit confused about #25, which states that the issue no longer affected D8 (13 years ago) and then there was a commit/fix in D9 3 years ago (in this issue #2936067: CSS aggregation fails on many variations of @import → , started 7 years ago). So we need to be sure what we are fixing here.

This logo was accepted and committed by the maintainer (see 📌 Project Browser: Add/update logo for XML sitemap Fixed ), so I think we can mark this as Fixed :)

Can you check this issue, if it will fix the problem? 🐛 [D7] preg_split in _filter_url breaks for long html tags Needs work

This does not seems like a D7 code. Moving to D11.

longwave → credited poker10 → .

It seems like the error output is caused by the fact, that the authorize.php is run with MAINTENANCE_MODE constant defined. That will cause that all errors are displayed unconditionally, see: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/includes/erro...

function error_displayable($error = NULL) {
  if (defined('MAINTENANCE_MODE')) {
    return TRUE;
  }
  ...
}

Therefore the warning is printed even if Drupal error reporting is turned off.

Even though there is a workaround mentioned in #3252394-31: JShrink incompatible with PHP 8 → , I think that it is worth fixing this as it is a small fix.

For others needing a newer jShrink, please check the approach mentioned in the comment #31 on the link above.

Committed and pushed this to 7.x-2.x, thanks!

Thanks for reporting a working on this. I think this looks good. Committed and pushed to 7.x-2.x.

poker10 → made their first commit to this issue’s fork.

Thanks for reporting this. I have tested the 8.x-1.0-beta1 with clean install of Drupal 10.3.1, Commerce 8.x-2.38 and Commerce Stock 8.x-1.3 and the \Drupal\commerce_stock_notifications\Plugin\Commerce\InlineForm\Notification::submitInlineForm() is executed as expected.

Do you have any other modules (contrib / custom) which can alter commerce or commerce stock somehow? I have tested this on PHP 8.1. Have not time to test it on Drupal 9.5. yet.

Thanks for reporting this. Can you provide more information - on which version of Drupal 7 it was working previously? And also have you upgraded your PHP version together with the Drupal upgrade? If yes, from which PHP to which?

Thanks for reporting this. I think this is something which needs to be changed in the svg_image module in the svg_image_field_widget_form_alter() function. Currently the module is altering the #upload_validators (see here).

In 🐛 Users are able to upload 0-byte images Fixed a new upload validator was added (see here):

+    $elements[$delta]['#upload_validators']['file_validate_is_image'] = array();

The module can remove the new validator to work as before.

Or you can use hook_field_widget_form_alter to remove it in the custom module as well.

@solideogloria according to this: https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintaine... → , each "Security update" release type should be linked to the correspoding SA. There are no SAs for these polyfill.io issues, see: https://www.drupal.org/psa-2024-06-26 → . Therefore I think that we should not use a Security update release type here.

mcdruid → credited poker10 → .

The failure is caused by changes from this issue: 🐛 Duplicate X-Content-Type-Options headers both with the value nosniff [D7] Fixed . The MR needs to be rebased and then it should be green again.

Great, thanks for the info! So closing this as a duplicate.

@frazac Not sure your warning is caused by the Drupal core, as it is in the ctools module. You can check a similar issue here: #1244434: Notice: Undefined index: handler type in ctools_context_handler_get_render_handler() (line 67 → , maybe it could help.

xjm → credited poker10 → .

Thanks for reporting this. Can you check if this patch would fix the problem for you? 🐛 advagg_js_compress() cron stuck issue Needs work

If we are considering to rename Drupal -> Drupal core and Starshot -> Drupal then we need to be careful. It was not the same situation (and we cannot compare it directly), but Home Assistant made a similar change in 2020, when initially the product was called just Home Assistant. After the rebranding, Home Assistant became Home Assistant core and Home Assistant itself was then a different product (the full OS). It caused a big confusion, problems with documentation, etc. Again, it is not the same situation, as Starshot and Drupal (core) both will be on the same core and the docs would apply to both of these, so I think more important will be how we will present this on the downloads page and so on.

Thnaks for creating the MR. I think we need to fix this in drupal_get_library(), not in drupal_add_library(), so that we cover all usages across core/contribs.

Added 7.101 and a new meta issue. Also fixed D9+ reference to D10+ reference.

I have created a new meta issue for the next release scheduled for 2024-12-04: 📌 [meta] Priorities for 2024-12-04 release of Drupal 7 Active and carried over all open issues from this meta issue.

poker10 → created an issue.

Drupal 7.101 was released as planned: https://www.drupal.org/project/drupal/releases/7.101 →

Thanks everyone who contributed!

Thanks for working on this. I have verified this is still an issue. I think we need to add a simple test here, but otherwise the fix looks good.

Thanks for working on this. According to the documentation, there are two required properties in the hook_image_effect_info(): https://api.drupal.org/api/drupal/modules%21image%21image.api.php/functi...

1. label
2. effect callback

All other are optional. Therefore you cannot omit the effect callback

Closing this as Works as designed.

I have tested this on a clean Drupal 7.101 and it does not seems to be an issue. Closing as Fixed for D8, so that credits are assigned correctly. Thanks!

Thanks for working on this. I think this is no longer relevant after: https://git.drupalcode.org/project/drupal/-/commit/24afdc00f534cf4c55a18... , which was committed on https://www.drupal.org/sa-core-2022-012 →

Thanks for working on this. Agree with @oriol_e9g, the patch #1 is not correct.

Thanks for reporting and working on this.

This is still used in D11 in ExtensionMimeTypeGuesser and a patch was against D8. Changing the version.

See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Thanks for working on this! According to this https://www.drupal.org/about/core/policies/core-change-policies/what-pat... → , we are not allowed to make changes in user-facing strings (or strings in error messages) in this D7 phase, unless these issues are critical/major:

Major/Critical string problem only. (e.g. typo, string is saying completely wrong information);

Therefore I must close this as Won't fix.

Closing this as the website seems to be no longer on Drupal. Thanks all.

Thanks for working on this.

Adding a related D10 issue. This is still Needs work, as the tests are missing. We can inspire with the D10 code if/where possible.

Thanks for working on this. This still Needs work, as there are open @todos. Also the patch does not apply anymore.

Thanks for working on this.

.htaccess is a specific file, which often contains some customizations and therefore is not always suitable to be used 1:1 on localhost. The code is commented and it is only an example/starter code. You still need to uncomment it, therefore you can make additional changes by adding the localhost exception directly on your site while uncommenting these lines.

I do not think we are going to make any additional changes to the .htaccess file in this D7 phase (except potential security-related changes). Given that the code is still the same in D11 as well, I think it will be the best to open a new issue for D11, if you still want to change this in the further Drupal versions. Thanks!

@nicxvan That is probably related to: 🌱 [meta] Deprecate tarballs, because they are incompatible with Composer-managed dependencies, Automatic Updates, Project Browser, and release managers' health Active

Thanks for working on this. There are contrib modules expecting the ['version'] array key from drupal_get_library(). I think the better approach is to return empty version in case the key is not present, but not omit the key, as the current patch #4 does.

The D8 issue was closed as Won't fix with this comment from @daffie:

For adding new datatypes to Drupal core they must work for all three by core supported databases. There is no enum datatype for SQLite.
I do not see how we can fix this for Drupal core. This should be fixed in a contrib module.

Therefore closing also this D7 issue. Thanks!

Thanks for catching this :) Agree that it would be good to update it.

Thanks for reporting and working on this. Yes, the user_hash_password($password); line is missing to correctly check 512 byte long password. Moving to RTBC.

I think this looks good. This is indeed an error, as the correct link is this:

  $items['admin/config/development/testing/results/%'] = array(
    'title' => 'Test result',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('simpletest_result_form', 5),
    'description' => 'View result of tests.',
    'access arguments' => array('administer unit tests'),
    'file' => 'simpletest.pages.inc',
  );

Moving to RTBC, thanks all!

This is a support request and there is no patch to review, so changing the status accordingly.

This is no longer relevant for D7, as the IIS config was removed from D11 as well, see: https://www.drupal.org/node/3440842 →

Closing this as fixed for D8, so that all contributors are credited correctly. Thanks all!

Thanks for reporting this. This was updated in D8 here: #2109065: Remove variable_get and variable_set calls from API documentation → . The next step should be to create a merge request with proposed changes. Thanks!

We just committed better error handling in a case the class is missing here #1982810: Core entity_get_controller gets a NULL controller class → . It should be now evident, which class is causing the problems and it should help users to identify a specific module, where to solve the issue. Closing this as outdated, but if this still a problem with Drupal core (not a contributed module), feel free to reopen. Thanks!

We just committed better error handling in a case the class is missing here #1982810: Core entity_get_controller gets a NULL controller class → .

I think we can close this support request as it was most probably caused by removing the enabled module which is not a good idea. Generally, this error is mostly caused by missing files or errors in contrib modules / custom code. You can look at the documentation of hook_entity_info to see that the there is already a fallback in case the controller class is not set - it defaults to DrupalDefaultEntityController. Checking if the given class really exists is not something Drupal core should do (it tends to not babysit broken code).

If you think this is still an issue for D7 core, feel free to reopen, but we would need a steps to reproduce this (without any non-standard actions). Thanks everyone!

Thanks for reporting and working on this. The patch no longer applies, moving to NW. Please create a MR if/when doing a re-roll.

Closing this as it seems like the support request was solved. Thanks!

Thanks for working on this. I think this was fixed by #3308929: [D7] Cron lock time limit is too short and does not prevent multiple, concurrent cron runs → . Cron lock is no longer a fixed value, instead its value is from a variable. Feel free to reopen if this is still an issue.

Adding credits from the duplicate issue.

Committed and pushed this, thanks everyone!

I do not think this applies to D10/11, as the arguments seems to be changed - both functions have only $account as a parameter now:

https://api.drupal.org/api/drupal/core!modules!user!user.api.php/functio...
https://api.drupal.org/api/drupal/core!modules!user!user.module/function...

Thanks! Committed and pushed this.

Committed and pushed. Thanks all!

Committed and pushed this. Thanks everyone!

Yes, I was on the edge when backporting these rows. Seems like we can use file_test_get_all_calls() to retrieve these results too, so I have updated the MR with this change. Hopefully now it is cleaner and safer. Thanks!

Tests are still green: https://git.drupalcode.org/project/drupal/-/pipelines/187591
And test-only job is still failing: https://git.drupalcode.org/project/drupal/-/jobs/1743067

Oh, sorry, I overlooked that the function is calling drupal_base64_encode() instead of standard base64_encode(). Then I think this is correct and the same as we have in D10. RTBC, thanks!

As there are still tests needed, changing the status.

Thanks for working on this. I see that D10 is replacing the characters (https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...), but D7 does not seems to do it (https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/bootstrap....). So is the comment correct?

 *   A base-64 encoded string, with + replaced with -, / with _ and any =
 *   padding characters removed.

Adding the tag for the final review.

I think this looks good. The comment was removed in D8+ and should be removed in D7 as well, as the original issue, which changed the behavior, was committed to the D7 14 years ago ( #605272-26: Use a better directory when installing code via the update manager (documentation followup) → ).

This proposed change was commited in D10 here: #1328696: Problem with _drupal_wrap_mail_line and attachment files → and then "reverted" here: #2078917: E-mails contain double spaces in soft-wrapped sentences →

I thinks this is now a duplicate of 📌 Erroneous signature and documentation for user_login_finalize() and hook_user_login() RTBC , where a MR is prepared. Thanks for working on this, I am closing this issue as there is no patch to review. We will assign credits if the second issue is committed.

Thanks for working on this!

Is this patch removing the safe_summary / safe_value data from specific text fields? If so, I think this is pretty disruptive for D7 in this phase.