🇦🇺Australia @acbramley

Feedback addressed

Another one in the same class 🐛 [random test failure] Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks Active

acbramley → created an issue.

Looks great, thanks

I have updated the Issue Summary, since your proposed solution results in an error message on the front page.

Well of course the idea is to change the default frontpage as well, the setting is required for a reason. What would be the result of your proposed solution? What would display on the frontpage in that case?

The views changes for the test point out there may be some BC issues here (assuming the same for the node change) but I suppose rendering either in views using the full view mode should be somewhat rare?

blocker is in

This is now fixed as of 📌 Deprecate $variables['page'] for node.html.twig Active being committed.

This is affecting MRs now https://git.drupalcode.org/issue/drupal-3015152/-/jobs/6259156

I've rolled #22 into an MR as that's the patch we've been using in prod for years. I like the idea of #60 but that'll have much wider reaching BC issues for sites using getParentEntity so I'm not sure how viable that'll be. Still NW for tests.

@shaila.amale please see #11 - this issue is closed.

Please contribute to the other issue, this one is closed 🐛 InvalidArgumentException: Invalid translation language (en) specified. in Drupal\Core\Entity\ContentEntityBase->getTranslation() (line 873 of core/lib/Drupal/Core/Entity/ContentEntityBase.php). Drupal\diff\Controller\PluginRevisionController->compareEntit Active

MR up following https://www.drupal.org/node/3442349 →

acbramley → created an issue.

Looking at the traits that implement a createEditorialWorkflow function, they don't seem to be overriding ContentModerationTestTrait. Many are ParagraphsTestBaseTrait.php files that don't have a return value at all. SchedulerUiTrait in lightning_scheduler isn't used in any tests alongside ContentModerationTestTrait. And finally ContentEntitySourceContentModerationTest in tmgmt doesn't interact with the core trait either. I think it'd be safe to add the return type to that function too. But I can also see the argument to not add it so I'll mark this RTBC.

Agreed that an empty tag should just be a no-op, it looks like adsense is using inline_template instead now. https://git.drupalcode.org/project/adsense/-/blob/8.x-1.x/adsense.module...

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

This is the same as CacheableMetadata::applyTo where you should merge metadata before using applyTo.

Something like:

$x = new BubbleableMetadata();
// Add stuff to $x
$y = BubbleableMetadata::createFromRenderArray($build);
$x = $x->merge($y); // or use $x->addCacheableDependency($y);
$x->applyTo($build);

Url::toRenderArray no longer exists 📌 Deprecate Url::toRenderArray() and Url::renderAccess() Fixed therefore the code in the IS is not possible anymore. As per #3, this was probably always the wrong way to do it anyway.

Closing this as outdated, if this is reproducible in other ways please feel free to reopen this issue and update the issue summary with those steps.

I think we should close either this or 🐛 Form API Table element is being treated as an input element, this can break fields that have a handler defined which are now no longer altering the data in the $form_state array. Needs work as they are duplicates.

Setting #input to FALSE seems like a valid fix for this, I suppose the only issue being that you can't have tabledrag then. I don't know if it'd be possible to support both though?

I've pushed a change so we don't need the new twig variable.

I'm also unsure if the CSS changes are needed anymore. I've tested both on Claro and Olivero with and without these changes and everything looks identical since we're simply allowing the form-required class to be added on the fieldset legend without the fieldset itself being required.

This is still valid on HEAD, tested with the default options checkboxes on the NodeTypeForm. AFAICT there is still no solution for this in raw HTML.

I followed the steps in the IS, both creating my own view and testing the sample config provided and neither view had the issue. I was able to copy the comment approve URL from the rest export (and remove the destination param and faulty encoded &) and the comment was approved successfully.

Looks like this may have been fixed in the past 5 years. Can anyone else confirm?

With the changes in 🐛 PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths Needs work the breadcrumb on /node/add becomes just "Home" and /node/add/foo becomes Home > Add content. IMO this is fine. The discussion about moving node/add/* under admin/content has been had elsewhere but I don't think there's a conclusion yet. As the node subsystem maintainer, I am heavily against moving it for similar reasons mentioned in #58

With that said, IMO this issue could be closed as a duplicate.

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

@smustgrave I don't see that text when using media library in CKE5, it probably got hidden at some point as that's more for fields with specific cardinality (it doesn't make sense when embedding media)

Rebased the MR and hiding patches to not confuse the bot.

This should be fixed in both 8.x-1.9 and 2.0.0-beta4. I recommend you update to the 2.x branch.

Tested this on HEAD by adding the following to Node::baseFieldDefinitions and running drush si standard -y, then checking the node_field_data table:

    $fields['foo__bar'] = BaseFieldDefinition::create('boolean')
      ->setLabel(t('Foo'))
      ->setRequired(TRUE)
      ->setDisplayConfigurable('form', FALSE)
      ->setDisplayConfigurable('view', FALSE);

The column existed as expected. The code mentioned in #13 no longer exists, instead it uses the DefaultTableMapping class to get the column names directly.

Can this issue be closed? Or is there another way to reproduce this?

This needs a reroll on an MR against 11.x and an IS update.

The test has passed 200/200 times. It's not clear from the previous issue where the failure was since the old CI jobs no longer exist so it's hard to tell if the random failure is indeed fixed

Had to remove the "item selected" assertions as that text no longer seems to exist in the CKE widget. Running the repeat test to see how many failures we get.

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

This needs a reroll into an MR against 11.x. Would also be good to get the IS updated with steps to reproduce.

Would be good to get a full list of steps to reproduce this.

I can't reproduce this, I think it would be dependent on file system permissions on the directory?

I created a fresh browser_output directory locally and ran a test and permissions were fine. Was able to delete the html output without sudo

adam@8fa2a3a71304:data $ ls -al app/sites/simpletest/browser_output/
total 68
drwxr-xr-x    2 adam     adam          4096 Aug 15 03:07 .
drwxr-xr-x  152 adam     adam          4096 Aug 15 03:07 ..
-rw-r--r--    1 adam     adam            55 Aug 15 03:07 .htaccess
-rw-r--r--    1 adam     adam          3567 Aug 15 03:07 Drupal_Tests_node_Functional_NodeAccessCacheabilityTest-1-84119625.html

I've tried to reproduce this with the Umami demo profile, when viewing the spanish homepage the contextual links go to the spanish translation. Are there different steps needed to reproduce this?

Popped up in BSI triage, it seems like this is still an issue based on the code in HEAD. Would be good to have some steps to reproduce this bug.

It looks like this would only happen if the field value contained a pid value that didn't have an associated path_alias entity. I think the defence would be good to add but we'll need some steps to reproduce here. I'm also not sure if creating the alias entity at that point is the right approach?

This needs a reroll on 11.x in an MR, the code has also changed a little bit (no jQuery) so the patch won't apply cleanly.

Might be good if someone can test this in Safari and see if it's still an issue?

This was fixed in #3320240: Entity count query returns a string instead of int →

See https://git.drupalcode.org/project/drupal/-/commit/c1891492e4d31cad222ba...

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

I checked all child classes of QueryInterface and they either cast result to an int, or use php's count() function which returns an int.

This needs a reroll into an MR, the easiest approach is to probably take #36 and apply it to a new branch against 11.x and hide the others as that seems to be the most up to date set of changes.

the Term parent field no longer uses custom storage (it's a regular ER field) so we should update the IS with an example (hopefully from core) that demonstrates the issue with steps to reproduce if possible

I think this should be PMNMI as we don't have steps to reproduce the bug.

This is no longer reproducible on HEAD because EntityForm::actions adds core/drupal.dialog.ajax to the delete link (as that opens in a modal). I guess it's probably a good idea to explicitly add this to ContentModerationConfigureForm but test coverage is going to be tricky.

Changing this to a Task since nothing is broken anymore

Needs a reroll into an MR and quite a bit of refactoring to be in line with modern practices (strict types, etc)

Do we have an example workflow type in contrib that would benefit from this? I.e workflowHasData returns TRUE when there are no entities provided by the module?

I agree this documentation is never going to be perfect if we continue to use ArrayPI here. Perhaps instead we need to swap to a value object to enforce these keys.

I tracked down where the absolute and https options are handled:

- absolute is set to FALSE by default in LinkGenerator and handled in UrlGenerator
- https is handled in UrlGenerator. It does not have a default value in UrlGenerator because if it's set to FALSE it forces http.

Agree with #3 that we should move attribute handling out of the Url object first.

Based on #7 it seems like this can be closed.

Please feel free to reopen if there is still a bug to be fixed here.

I tried to reproduce the install/uninstall error in https://www.drupal.org/project/drupal/issues/2743175#comment-16228964 🐛 SqlContentEntityStorageSchema::getDedicatedTableSchema assumes that a fields property definitions matches the columns Needs work to check if this was a duplicate but I'm unable to reproduce the error anymore.

Tried to reproduce this on HEAD using a base field on Node:

1. Install standard
2. Add the following to node.post_update.php:

function node_post_update_foo(): void {
  $field_definition = BaseFieldDefinition::create('map')
    ->setLabel(t('Foo'))
    ->setRequired(TRUE)
    ->setDisplayConfigurable('form', FALSE)
    ->setDisplayConfigurable('view', FALSE);
  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
  $definition_update_manager->installFieldStorageDefinition('foo', 'node', 'node', $field_definition);
}

3. Add the following to Node::baseFieldDefinitions:

    $fields['foo'] = BaseFieldDefinition::create('map')
      ->setLabel(t('Foo'))
      ->setRequired(TRUE)
      ->setDisplayConfigurable('form', FALSE)
      ->setDisplayConfigurable('view', FALSE);

4. Run drush updb -y

No errors, the field is added to the node_field_data table

5. Try the opposite:

function node_post_update_bar(): void {
  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
  if ($field_definition = $definition_update_manager->getFieldStorageDefinition('foo', 'node')) {
    $definition_update_manager->uninstallFieldStorageDefinition($field_definition);
  }
}

No errors, the field is removed from the node_field_data table.

Has this been fixed elsewhere?

Based on our issue category definitions I don't think this should be considered a bug.

It's not clear to me how to reproduce this, but I tried installing from standard profile on HEAD and enabling the settings_tray module. I then used the quick edit contextual link on several different blocks on the homepage and they were always full height.

Triaged as part of bug smash. It doesn't seem like this is a bug and I'm not actually sure if it'd be possible (or beneficial) to combine the 2.

Element::isVisibleElement is a static function so doing anything with access_callback would be tricky, and it may be intentional that this code doesn't try to process that callback. That would mean isAccessibleElement would just be checking something like ($element['#access'] instanceof AccessResultInterface && $element['#access']->isAllowed()) || ($element['#access'] === TRUE) but then ::doRender needs to do those checks anyway so it can add the cacheable dependency, etc.

With that in mind, I'm not sure if this is something we need to do?

@alexpott that's exactly what ContentEntityStorageBase::createRevision is documented to do from what I'm reading. Without this fix, reverting a Node to a more recent revision that is not current (i.e clicking Set as current revision) has a really weird outcome and essentially bugs out the revision list (try running the tests without the fix and looking at the HTML output or debugging yourself).

I'd recommend to create a separate branch for 2.x, people are going to need to 8.x-1.x version for quite some time, kinda too late, but you could also re-open an 8.x-1.x MR based on a patch or something I suppose.

There are 2 MRs open, one against 8.x-1.x and one against 2.x. The 8.x-1.x one is rerolled against HEAD.

acbramley → created an issue.

IMO this should be closed as works as designed. We certainly cannot suddenly start exposing all orphaned files on existing sites to the public.

Given this is how private files have worked for ~15 years or more, I don't think we should change this in core. As per #1, you can change this in a contrib or custom module using hook_file_download.

I think I'm missing something, but why exactly do we want to combine all CSS into a single file? Currently these CSS files are split into separate libraries (although node.module.css is in 2 different libraries) which are added in different parts of the CMS depending on where they're needed. Combining all CSS into 1 file will mean adding CSS to places that it's not needed. Isn't it better to only add it where it's needed?

We also have many themes overriding or extending these libraries to add their own CSS, we would have to very carefully coordinate releases between all of these otherwise there will be visual regressions. Furthermore, it means contrib would have to have a new major/minor targeting the specific core release this is changed in.

The effort doesn't seem worth it IMO, what are we trying to gain here? Even though the CSS is all admin-ish related, it's for very different use cases.

Rebased on top of 2.x. I was going to suggest using a compiler pass to auto register the client factories inside a dedicated namespace so we don't have to manually declare each one in services.yml, I might open a new issue for that.

Still TBC how we solve the pipeline issue elegantly, especially with things like getMultiple returning from the exec command which isn't supported by default on the cluster client.

Tests are failing and there seems to be a lot of unrelated changes in there.

This also seems likely to have overlap or duplicate with these issues that are all postponed on fixing a related issue in core, once the core issue is fixed we will apply the same logic to Diff.

#3227907: Current revision listed only for one translation →
🐛 Revisions controller getRevisionIds() should filter by language Needs review
🐛 Revisions are not visible for some nodes Needs review

Core issue 📌 Consider showing all revisions on the revision overview Active

Closing this for now as it's not clear to me what the goal is of this issue. Feel free to reopen it with clearer scope in the IS.

Updating credits.

Given all the points raised in #27, especially the potential performance impact and the fact that this is so easy to toggle on for a site I think it's best that we leave it off by default.

acbramley → changed the visibility of the branch 2877924-make-node-aware to hidden.

Confirming we can get rid of all of this code once 📌 Deprecate $variables['page'] for node.html.twig Active is done, however CM doesn't seem to have any functional test coverage of what this was doing (i.e hiding the duplicate h2 heading on the latest version page).

I think in this issue we should add that test coverage. I've opened an MR to remove the associated code and will add the test coverage which will fail until 3458589 is merged and this is rebased.

I can't reproduce this on HEAD, what version of Drupal are you running? And what links are you clicking on?

I've tested with both adding and editing a node and clicking on a toolbar link.

Latest commit on https://git.drupalcode.org/project/redis/-/merge_requests/61 has broken cluster integration due to the forced exec. I can't see a way around it at the moment so I've just rerolled the original MR for now.

Rebased this and fixed up a bunch of conflicts, copying across some changes to hook_help that made it in in the meantime.

I don't think the new interface method would fly even though it's probably the cleanest approach. I've swapped to a static method on NodePreviewForm

Contributions must be made via MRs, not patches. And we'll need tests as per the issue tags.

Is there any reason you can't update to 2.x?

Given the lack of activity here and that this method is only a handful of lines long I think it's best to keep it protected. More public API means more maintenance and BC ramifications.

If you feel strongly about this, feel free to reopen it with more concrete examples of what this feature would provide.

I have taken the same approach as 📌 Remove BlockContentController::Add Needs work here and reversed the aliases as well as removed the deprecations to unblock this issue.

This can be reversed again in 📌 [PP-2] Remove route renames and aliases for node Active

acbramley → created an issue. See original summary → .

MR had failures and wasn't rebasing cleanly so I've squashed and rebased against 11.x

https://git.drupalcode.org/project/redis/-/merge_requests/62 is an initial approach to refactoring the cluster client (PhpRedisCluster only, not Predis) using the new architecture in 3080449

It would be great if anyone wants to test that branch and report back if it's working for you :)

Tested this and put together a cluster version. Using docker-compose and https://github.com/Grokzen/docker-redis-cluster

https://git.drupalcode.org/project/redis/-/merge_requests/62

Docker container:

  redis:
    image: grokzen/redis-cluster:latest
    network_mode: service:nginx
    environment:
      IP: '127.0.0.1'

Nginx ports:

      - '7000-7005:7000-7005'
      - '5000-5010:5000-5010'

It seems to be working well, info page is showing similar info to 1.9 + cluster patch.

This came up in BSI random triage. #40 still applies.

Seems to be a non random test failure.

I'm also not sure why we need to do this? These changes would the basic block in standard and the basic_block_type recipe basically useless (it will have no fields). We didn't remove the body field from article or page content types for Node and I don't think we should.

We should change them from text_with_summary though

What is the more “modern” recommended way to implement these features if these are to be removed from Core?

There is nothing wrong with using the base fields, these will always be available via a contrib module when they are removed from core (in a major release).

Still an issue on 11.x for base field definitions. #11 still works :)

jibran → credited acbramley → .

I think we need to postpone this on 📌 Introduce Adapters instead of different cache/queue/lock classes per backend Active

Woops :D confirmed that this is valid - another reason to try and get our phpstan levels higher.