🇸🇰Slovakia @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.

I have reviewed the MR and it looks good to me. Compared with the #2, the $this->tearDown(); is also moved to the try-catch block, but I think this solution is better, because we probably want also tearDown() to run without any exceptions. If there are any problems, it would be better to stop the execution with the exception, instead of "seeing what will happen" next.

Tests are also green. Adding a tag. Thanks all!

Thanks for working on this. I reviewed the patch and compared it with the D10. I am curious, if there is an issue for this for D10? Because it seems like the code in D10 is the same as the D7 code:

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

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

Do we need to change this in D10 first?

Thanks!

Thanks for working on this! I have reviewed the patch from #76 and I think the tests are still missing and need to be backported. Changing the status.

We can probably keep it in this issue if the tests are only one thing missing, but if not, the correct approach would be to move this to a separate issue. It would be also great to create a MR instead of a patch. Thanks!

@Andrei Haurukovich can you verify if this is an issue on D10 as well? Thanks!

Adding a tag for the final review.

Adding a tag for the final review.

Adding a tag for the final review.

I have drafted a CR here: https://www.drupal.org/node/3451532 →

Also moved the patch to the MR so that we are sure it is still green: https://git.drupalcode.org/project/drupal/-/pipelines/187106

Adding a tag for the final review.

I have drafted a simple CR here: https://www.drupal.org/node/3451526 → (feel free to update if needed).

Otherwise I think this is ready. Adding a tag.

Adding a tag for the final review.

The list of attendees looks good to me. Thanks.

benjifisher → credited poker10 → .

@ChrisScrumping yes, it would be great if you can update the MR and prepare it for review. I can take a look then a finish it / commit.

I would say we can keep old core versions and old php versions in case it does not do any harm (maybe it will be less disrupting for sites).

Thanks!

poker10 → credited poker10 → .

poker10 → credited poker10 → .

poker10 → credited poker10 → .

poker10 → credited poker10 → .

greggles → credited poker10 → .

I think that Nginx Unit and Nginx + PHP-FPM are two different solutions and neither one is going away. If anything, we should probably still target classic Nginx + PHP-FPM config, which should be used by majority users these days (but I have not found relevant usage statistics for Nginx Unit).

This was mentioned on Slack (but from the security perspective, not directly related to these decisions), but my personal opinion on this is a bit different from @cmlara's (especially when I put aside a bit the security perspective and the mentioned risk).

@jitesh_1 got the maintainership approx. a year ago, see: #3335545: Offering to maintain Simple Popup Blocks → . He have done one commit: https://git.drupalcode.org/project/simple_popup_blocks/-/commits/8.x-2.x .

There is a guide what is the responsibility of a maintainer: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... → .

So question for @apaderno is - should @jitesh_1 be able to "block" another help offer in this case? I am not entirely sure. Especially taking into account these circumstances and looking at the profile of @i-trokhanenko and @jitesh_1. But yeah, the final decision is in the hands of the moderators.

Thanks for reporting this.

I think the main reason we skipped this additional code was, that the D9+ code has the same implementation without the event.isDefaultPrevented() (see the current Drupal 11.x-dev code: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/form.js). There is an issue for D10 still open #3251249: Should double-click prevention return early when isDefaultPrevented? → . I think we need to decide and solve this for D10 first. If there will be a decision to add this code, we can add this to D7 as well. Without it, I am not sure if we are going to change a way how it works, as it will be a bit different in D7 and D10 then. So if possible, please focus on the D10 issue and then we can mode here as well. Thanks!

mcdruid → credited poker10 → .

Yes - both fixes are in MR 8020 to see the final effect (but this MR will not be committed) and then there are two separate issues with MRs to commit.

I have done it because the MR workflow is more flexible and also because I was not able to simulate the session test failure locally, so I needed to experiment a bit.

Hiding the old patch as we have now a MR here for testing.

I have created another child issue for the session failure.

testEmptySessionId() failure: 🐛 [D7 PHP 8.3] Fix SessionHttpsTestCase->testEmptySessionId() failure Needs review

testLength() failure: 📌 [D7 PHP 8.3] TextSummaryTestCase::testLength() fails on some libxml versions Active

Created also a draft MR here with PHP 8.3 testing (https://git.drupalcode.org/project/drupal/-/merge_requests/8020) and after applying fixes from both issues, tests are green, see: https://git.drupalcode.org/project/drupal/-/pipelines/170105 (sorry for multiple commits, but I had/have issues with replicating the session failure locally).

I tried to reproduce this failure on multiple enviroments: local (linux, windows), our demo server, .. and was unable to reproduce it anywhere, so was unable to debug it. I think it could probably be related to some PHP enviroment config.

While trying to debug the SessionHttpsTestCase->testEmptySessionId() test, I found out, that the test seems not working at all.

I have created a test branch with a test MR: https://git.drupalcode.org/project/drupal/-/merge_requests/8025 . I have reverted the SA-CORE-2014-006 security fix in session.inc there. Then I expected that the SessionHttpsTestCase->testEmptySessionId() test would start failing. And it does not, it is still green, see: https://git.drupalcode.org/project/drupal/-/pipelines/170033

Therefore I think the test is currently not working and it is questionable, if it was working correctly in the past.

----------

So I think that the one solution is to remove the non-working test (not created a MR yet). Other possible solution is to use something similar as it is used in SessionTestCase::testEmptySessionID() - add a $this->curlClose(); just before the drupalGet() call. This should at least fix the failure on PHP 8.3.

I have created a MR 8029 for the second possible solution here: https://git.drupalcode.org/project/drupal/-/merge_requests/8029

All current tests seems to be green with this change: https://git.drupalcode.org/project/drupal/-/pipelines/170155 . Also the PHP 8.3 test is without this failure (only the HTML parsing failure from TextSummaryTestCase() is still present after applying the fix from here), see: https://www.drupal.org/pift-ci-job/2893116 → .

For illustration, if we combine fix from this issue and fix from the other one, the 8.3 testing is green: https://git.drupalcode.org/project/drupal/-/pipelines/170105 (see the META issue).

Moving to Needs review.

We discussed this issue with @Fabianx on Slack and we agreed it would be the best to just skip the testing of this edge-case for the LIBXML version >= 2.9.14. It seems like the change in libxml2 have not caused other issues with D7, as other tests are not failing.

I have created a MR here with the proposed fix (it just skips the testing of "<" when tested with filter_htmlcorrector in case the LIBXML version is >= 2.9.14. It was possible to do this in several ways, but I think this approach has the smallest impact (unfortunatelly it seems not easily possible to remove the array element entirely, as the for loop is testing the lenght by array key position).

All current tests seems to be green with this change: https://git.drupalcode.org/project/drupal/-/pipelines/170137 . Also the PHP 8.3 test is without this failure (only the session one is still present after applying the fix from there): https://www.drupal.org/pift-ci-job/2893113 → .

For illustration, if we combine fix from this issue and fix from the other one (session failure), the 8.3 testing is green: https://git.drupalcode.org/project/drupal/-/pipelines/170105 (see the META issue).

Moving to Needs review.

poker10 → created an issue.

D7 also needs fix

Actually, as D7 still has to support PHP 5.6 and PHP 7.0, we would probably need to drop type declarations where needed and add type checks to the functions itself, see: https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullabl...

Because nullable types were added only in PHP 7.1.

But thanks for the initial research. I will create a separate issues for D7 soon.

There is an unrelated MR pipeline failure. It seem to be related to this: 🐛 Composer tests fail because of invalid Drupal version Fixed

Therefore keeping this as Needs Review.