Account created on 20 June 2006, over 17 years ago
  • Drupal Core Committer at AgileanaΒ  …
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

I reviewed all the uses of url_alter_test in core and agree it can be removed. The fiddling with Forum's weight was added in #320331-69: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks β†’ , as in, well before the release of D7, when everything to do with URLs, routing, and module weights/dependencies was wildly different. That comment reads:

Ok this one is almost ready. Still trying to figure out what's going wrong with the wonky forum tests...

Some other quotes from that issue:

In IRC, Dave and I determined the source of the badness is that calling module_list(TRUE, TRUE) to refresh the module list from inside a reduced bootstrap level (we're still just at DRUPAL_BOOTSTRAP_PATH when drupal_path_initialize() runs) results in sheer badness. entity_get_info() is confused, we have an empty list of modules (or something), and we end up caching an empty array in {cache}.cid == 'entity_info', resulting in a completely broken site until you manually clear that cache entry. :(

So:

A) WTF, module_list()? Why can't you be refreshed twice during the bootstrap process?

B) WTF, entity_get_info()? Why are you willing to cache an empty array into the DB? ;)

For now, this patch is going to work-around it by calling module_list(FALSE, TRUE) to only get bootstrap modules, but *not* refresh the list.

Forum.module is very broken. Very upset and frustrated.

And to properly compare old vs new, make sure we are using worst-case scenario. Locale.module enabled and altering language prefixes (a non-default language path), and also using taxonomy_term_path() with forum.module.

So overall I think this is ready, but NW for the followup I mentioned in #12.1.

πŸ‡ΊπŸ‡ΈUnited States xjm

Just documenting my own results while I review.

Before

[ayrton:drupal | Sun 10:33:07] $ grep -ri "forum" * |  grep -v "node_modules" | grep -v "vendor" | grep -i "tests" | grep -vi "migrat" | grep -v "core/modules/forum" | grep -v "fixtures"
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:      case 'field.field.node.forum.comment_forum':
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->enableModules(['field', 'node', 'comment', 'taxonomy', 'forum']);
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->assertNull(FieldConfig::load('node.forum.comment_forum'));
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->installConfig(['forum']);
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->assertNotNull(FieldConfig::load('node.forum.comment_forum'));
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:    yield 'Dynamic type with [%parent.%parent]: field.field.node.forum.comment_forum:default_value.0' => [
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:      'field.field.node.forum.comment_forum',
core/modules/system/tests/modules/url_alter_test/url_alter_test.install:  // Set the weight of this module to one higher than forum.module.
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:  protected static $modules = ['forum'];
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:    'forum_index' => ['created', 'last_comment_timestamp'],
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:      // enabled modules: forum, language, locale, statistics and tracker.

After

[ayrton:drupal | Sun 10:35:06] $ grep -ri "forum" * |  grep -v "node_modules" | grep -v "vendor" | grep -i "tests" | grep -vi "migrat" | grep -v "core/modules/forum" | grep -v "fixtures"
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:  protected static $modules = ['forum'];
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:    'forum_index' => ['created', 'last_comment_timestamp'],
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:      // enabled modules: forum, language, locale, statistics and tracker.

Per the IS:

core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php - no change, An update test using forum that will be removed in 11.0

  1. Is there an issue for this? If not, we should file it parented to the D11 beta blocker meta or one of its children. (And add to related issues here.)
  2. This is out of scope, but: Do we have other tests of 2038 upgrade paths? I guess the idea is that people must upgrade to 10.3/10.4 first regardless, which would already require their timestamps to be updated, and so it's not needed in D11. But it seems weird to have the test of this coupled only to Forum.

I diffed the Forum and new test fixure config:

[ayrton:drupal | Sun 10:49:13] $ diff -uP core/modules/forum/config/optional/field.field.node.forum.comment_forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/field.field.node.config_mapping_test.comment_config_mapping_test.yml
--- core/modules/forum/config/optional/field.field.node.forum.comment_forum.yml2023-09-25 18:03:35
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/field.field.node.config_mapping_test.comment_config_mapping_test.yml	2024-01-14 10:44:22
@@ -2,14 +2,14 @@
 status: true
 dependencies:
   config:
-    - field.storage.node.comment_forum
-    - node.type.forum
+    - field.storage.node.comment_config_mapping_test
+    - node.type.config_mapping_test
   module:
     - comment
-id: node.forum.comment_forum
-field_name: comment_forum
+id: node.config_mapping_test.comment_config_mapping_test
+field_name: comment_config_mapping_test
 entity_type: node
-bundle: forum
+bundle: config_mapping_test
 label: Comments
 description: ''
 required: true
[ayrton:drupal | Sun 10:50:43] $ diff -uP core/modules/forum/config/optional/field.storage.node.comment_forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/field.storage.node.comment_config_mapping_test.yml
--- core/modules/forum/config/optional/field.storage.node.comment_forum.yml	2023-01-16 08:10:14
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/field.storage.node.comment_config_mapping_test.yml	2024-01-14 10:44:22
@@ -4,12 +4,12 @@
   module:
     - comment
     - node
-id: node.comment_forum
-field_name: comment_forum
+id: node.comment_config_mapping_test
+field_name: comment_config_mapping_test
 entity_type: node
 type: comment
 settings:
-  comment_type: comment_forum
+  comment_type: comment_config_mapping_test
 module: comment
 locked: false
 cardinality: 1
[ayrton:drupal | Sun 13:14:50] $ diff -uP core/modules/forum/config/optional/node.type.forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/node.type.config_mapping_test.yml
--- core/modules/forum/config/optional/node.type.forum.yml	2024-01-14 10:14:20
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/node.type.config_mapping_test.yml	2024-01-14 10:44:22
@@ -3,10 +3,10 @@
 dependencies:
   enforced:
     module:
-      - forum
-name: 'Forum topic'
-type: forum
-description: 'A <em>forum topic</em> starts a new discussion thread within a forum.'
+      - config_mapping_test
+name: 'config_mapping_test topic'
+type: config_mapping_test
+description: 'A <em>config_mapping_test topic</em> starts a new discussion thread within a config_mapping_test.'
 help: null
 new_revision: false
 preview_mode: 1
[ayrton:drupal | Sun 13:17:26] $ diff -uP core/modules/forum/forum.info.yml core/modules/system/tests/modules/config_mapping_test/config_mapping_test.info.yml
--- core/modules/forum/forum.info.yml	2023-09-25 18:03:35
+++ core/modules/system/tests/modules/config_mapping_test/config_mapping_test.info.yml	2024-01-14 10:44:22
@@ -1,12 +1,8 @@
-name: Forum
+name: Config mapping test
 type: module
-description: 'Provides discussion forums.'
+description: 'Test configuration mapping.'
 dependencies:
   - drupal:node
-  - drupal:history
-  - drupal:taxonomy
   - drupal:comment
-  - drupal:options
 package: Core
 version: VERSION
-configure: forum.overview
πŸ‡ΊπŸ‡ΈUnited States xjm

Also, this issue seems to address docblock comments, but there are still a number of inline comments that also reference the book module:

[ayrton:drupal | Sun 10:18:18] $ grep -ri "\bbook\b" * | grep "//" |  grep -v "core/modules/book" | grep -v "vendor" | grep -v "node_modules" | grep -vi "migrate"
core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php:    // Simulate starterkit_theme defining the book-navigation library.
core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php:    // Activate the theme that extends the book-navigation library in
core/lib/Drupal/Core/Database/Connection.php: * @see http://php.net/manual/book.pdo.php
core/lib/Drupal/Core/Password/PhpPassword.php: * @see https://www.php.net/manual/en/book.password.php
core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:        // (cf. http://www.libpng.org/pub/png/book/chapter08.html#png.ch08.div.2)
core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php:    // Make sure the book node exists.
core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php:    // Ensure that the Book module's node type does not have duplicated enforced
core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php:      $descriptions[] = $this->t('The GD Library for PHP is enabled, but was compiled without support for functions used by the rotate and desaturate effects. It was probably compiled using the official GD libraries from the <a href="https://libgd.github.io/">gdLibrary site</a> instead of the GD library bundled with PHP. You should recompile PHP --with-gd using the bundled GD library. See <a href="https://www.php.net/manual/book.image.php">the PHP manual</a>.');
core/modules/views/tests/src/Kernel/TestViewsTest.php:    // `node.type.book` config entity is a config dependency.
core/modules/views/tests/src/Kernel/TestViewsTest.php:    // `node.type.book` config entity is a config dependency.

Some of those are not relevant, but others definitely reference the book module.

Saving credits.

πŸ‡ΊπŸ‡ΈUnited States xjm

Also, I looked at the remaining usages and I think we should also update this bit from comment.module:

* Users can post comments to discuss a story, collaborative book page, user    
 * etc.  
πŸ‡ΊπŸ‡ΈUnited States xjm

This looks good overall. I left one suggestion on the MR for a comment that went weird, and another question.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm

One thing to keep in mind here is that some of these issues might have been intended for backport, so we should spot-check issues for that and just close them if no backport will be done. Here are the RTBC ann PTBP issues against D9 or D10 branches, which should be triaged:
https://www.drupal.org/project/issues/search/drupal?project_issue_follow... β†’

πŸ‡ΊπŸ‡ΈUnited States xjm

Thanks everyone! Now that the GitLab discussions are linked from the IS, the discussion should continue there and not on d.o. Thanks everyone!

πŸ‡ΊπŸ‡ΈUnited States xjm

Amending attribution.

πŸ‡ΊπŸ‡ΈUnited States xjm

@klausi and I discussed this issue a little more in DM and I pointed out in a bit more detail why this discussion needs to move off d.o and particularly out of the core ideas queue.

Now that the community has consolidated around using a GitLab project space, let's update the issue summary with a link to a planning issue in the D7Security project, and then we'll close this issue. Hopefully folks interested in this topic can continue the discussion over there. Thanks everyone!

πŸ‡ΊπŸ‡ΈUnited States xjm

Backdrop is not a Drupal product. It's a Backdrop product and it's not Drupal core's responsibility to document anything about it. They already document their own merits: https://backdropcms.org/

Closing (won't fix) as per #3.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm

Grammar oddity

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

Have to choose between bold and borders, or switch to Full HTML which would prevent editing by most people.

πŸ‡ΊπŸ‡ΈUnited States xjm

Add explanation about the three release windows WRT previously posted concerns.

πŸ‡ΊπŸ‡ΈUnited States xjm

.

πŸ‡ΊπŸ‡ΈUnited States xjm

oops

πŸ‡ΊπŸ‡ΈUnited States xjm

Updates for the release of 10.2.0.

πŸ‡ΊπŸ‡ΈUnited States xjm

Why would we not use the command that allows Macs to run 8 processes in parallel? I don't see what the downside is of including that command i in the condition chain.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ changed the visibility of the branch 3408932-update-phprequirements-with to hidden.

πŸ‡ΊπŸ‡ΈUnited States xjm

Adding missing PhpRequirements child.

πŸ‡ΊπŸ‡ΈUnited States xjm

Actually this did happen and just was not in the meta (also I was looking at the wrong branch):
πŸ“Œ Update list of PHP EOL dates Fixed

πŸ‡ΊπŸ‡ΈUnited States xjm

Just confirming that @quietone should have access to unpublished blog posts (and possibly other similar content access bundled with whatever approriate d.o role). She is a Drupal core release manager, and as such is already trusted with the integrity of the entire core project. Being able to create, view, and edit unpublished Drupal core blog posts is a needed part of our release management responsibilities. Thanks!

πŸ‡ΊπŸ‡ΈUnited States xjm

Amending attribution.

πŸ‡ΊπŸ‡ΈUnited States xjm

xjm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

Saving credits.

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

Added the draft snippet. It can be iterated on when we create the CR and when we finish all the conversions for 10.3.

πŸ‡ΊπŸ‡ΈUnited States xjm

And I guess a lightweight CR for that matter.

πŸ‡ΊπŸ‡ΈUnited States xjm

Belated "oopsie" as the committed changes should have been tagged for release notes due to the impact on tests and the new phpcs rules (it is my fault for not catching this). We'll need a similar note again in 10.3 for the rest of the conversions, so adding it here in a minute..

πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm
πŸ‡ΊπŸ‡ΈUnited States xjm

Yes #22 is the "oh duh why didn't I think of that" robustness that we should have.

πŸ‡ΊπŸ‡ΈUnited States xjm

I'd like to ask that folks limit further discussion on this issue to figuring out where this effort will be hosted, and once that decision is made, the discussion can move there (and out of the core ideas queue, since the core committers generally will not be responsible for this effort).

πŸ‡ΊπŸ‡ΈUnited States xjm

@xmacinfo, I think that's out of scope here, but rest assured the DA is working hard on getting d.o upgraded -- it's one of the things that makes the GitLab integration such a high priority -- and that d.o won't be left insecure regardless of what else happens. Infra, the core committers, and the Sec Team all work together when needed, and the lead developer for d.o is also on the Security Team.

πŸ‡ΊπŸ‡ΈUnited States xjm

The MR should be made against 11.x FWIW (which means filing the issue against 11.x too nowadays). We'll need to decide based on the eventual solution how far it's safe to backport it. This issue sounds bad enough that could even be backported as far as 10.1 (at release manager discretion) if we are able to come up with a safe fix.

πŸ‡ΊπŸ‡ΈUnited States xjm

@klausi I was being a bit bombastic; what I really meant was "every effort is made to ensure that people do not incorrectly contact the DA/Security Team/uninvolved core or contrib maintainers or post issues in Drupal.org queues". I.e., it should be a part of the project's README and processes to help people use the correct channels and not use Drupal.org/contributor resources.

πŸ‡ΊπŸ‡ΈUnited States xjm
Production build https://api.contrib.social 0.61.6-2-g546bc20