🇸🇰Slovakia @poker10

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.

Actually after the suggested title change, it may look like this is no longer a "Bug report", but I suggest this is still treated as a bug, because it is still a broken functionality related to D7 -> D10 upgrade (and pretty major).

I have updated the comments in the MR (still open to further suggestions), issue title and the CR.

I have not used session_name (with underscore) in those sentences, but instead used session name, because from what I have seen, we use this phrase without underscore on different places in code already (phrase session_name is only used as a $session_name variable). So I think it would be better without the underscore.

Also agree with @cilefen, that the name_suffix is probably sufficient, as it is under the session.storage.options.

Thanks!

Thanks all for working on this! I have converted the latest patch to the MR. Also updated the name as per #69.

Draft CR is here: https://www.drupal.org/node/3446100 →

I think this issue is pretty important to fix, as with the D7 EOL approaching, this has potential to cause a lot of troubles to sites still going to migrate. We were also caught by this on a few sites and no user with a D7 cookie was able to login until the cookie was deleted.

Pipeline seems to be green, moving to Needs review.

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

Thanks for great work @bradjones1! I have added some other comments to the MR.

There was a test failure caused by a change in this issue 🐛 Duplicate X-Content-Type-Options headers both with the value nosniff [D7] Fixed - the rebase was needed. Now tests are green.

I think this looks good. Adding a tag for a final review. Thanks!

benjifisher → credited poker10 → .

Sorry if this was explained somewhere else, but what exactly does the "Stable" and "Maintained" means?

Does the stability status relate to your installed branch (or in case the module is not installed to the newest branch)? Because a project can have a different stability statuses on various branches.

Is the "Maintained" just a status taken from the project configuration, which is misconfigured in a lot of projects and does not reflect reality? Or is it a list of maintainers?

Thanks!

Thanks for reporting and working on this.

In Drupal 7 there is no UI to manage form display (similar as in D10). So most of the sites are using hook_form_alter() to move form elements up/down and achieve the changes in the order. Maybe there are also contrib modules doing something similar.

I agree this is a bug, but I think we cannot change the default weights in this D7 phase, because a lot of sites simply rely on the current settings (and have done custom changes based on this default settings). I would say there is a way more sites using hook_form_alter() to move form elements than sites with 100+ node fields. And this change would cause their node forms to re-arrange, so it could have a big impact. If someone wants to change this, it is easy and can be done using the hook_form_alter().

Closing this as Won't fix. Thanks again for your effort here.

Thanks all for working on this.

I think we should move with the D7 MRs/patches to the newly created/separate issue as per current backport policy. The last D7 patch still has a work to do - at least the tests are not backported. Also it is questionable if we should use REQUIREMENT_ERROR in this D7 phase, or lower it to REQUIREMENT_WARNING.

I am closing this issue as Fixed for D8. D7 backport issue is here: 🐛 [D7] HTTP_HOST header cannot be trusted Needs work . Thanks.

Adding a last D7 patch from the parent issue, but let's convert it to the MR and add a missing tests.

poker10 → created an issue.

Thanks for working on this!

Should we also rename the $edit parameter in hook_user_login() to get it done completely?

Taxonomy node filter was removed in #693362: taxonomy_form_all() is dangerous → and it seems like this was left here unintentionally.

I have done a quick search on the Drupal codebase via Gitlab a it seems like no contrib module is setting the $_SESSION['node_overview_filter'] variable, which should be the only way how term could end up here. Therefore I think this is safe to remove.

Committed, thanks all!

Thanks all for working on this!

Testing core gate now has an exception, specifying when tests are not needed, see: https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . One of the conditions is that

"The issue has clear ‘Steps to reproduce’ in the Issue Summary."

.

So my question is - beside the manual table creation - do we know a way how we can achieve that the table is created, exception is thrown only afterwards and therefore the table is not deleted correctly? What other exceptions can be thrown after the table is created and before it is dropped? If there are some possibilities, then great. If not, should not we consider an alternative approach to check and drop the table in the catch block instead?

I have looked at all CREATE TABLE statements in D7 core and none of these is using IF NOT EXISTS.