🇦🇺Australia @acbramley

Rebased again and fixed the performance tests.

MRs need to be against the 11.x branch. Also hiding all the patches.

Converted to use a memory cache

Fixed, since this is a trivial change I'm going to self RTBC.

I can't see any help text on the block types page and searching for "custom block libraries" brings up nothing so it's hard to tell what #34.2 is referring to.

We just need to get 📌 Convert ckEditor5EditorHeightTest to WebDriver test Active in

Going to self RTBC this since it was only set back for merging the test changes which were fixed with https://git.drupalcode.org/project/drupal/-/merge_requests/352/diffs?com...

I've manually reproduced this issue and looked into how group (or more explicitly the gnode) module works with these routes as well as skimmed through ✨ Node previews don't work when adding group node Needs work .

From what I can see, gnode implements its own routes and forms to achieve this functionality and therefore should be in charge of altering the url for the back link.

This issue mentions functions that no longer exist. Let's create a new issue with more detail if anything still needs fixing.

Given this hasn't had any meaningful activity in 11 years I'm marking this as a won't fix.

I can see many UX issues with what is being proposed in the IS, mainly around different bundles having different fields and data loss.

If anyone is interested in implementing this, I would recommend doing so in contrib and then creating an issue to get buy in for moving the functionality into core.

The Create new revision checkbox is no longer tied to the administer nodes permission.

Permissions are grouped by module name and Node obviously won't be changed. Content has a much broader scope now that we have content entities. This is a won't fix I think.

~14 years since any activity on this one, I think it's safe to close out.

This setting is not designed to have any relation to the revisions table. Hiding the date and author information in the table would effectively hide any relevant information. This would likely be undesirable for existing sites which have this setting disabled.

I can't think of a feasible way to do this given the unknowns of both the teaser display and the full display of any given node. Given the age of the issue I'm going to close this.

The /node page is provided by the frontpage view, the permission to see that path can be modified in the view itself. This is outdated as a new permission doesn't make sense anymore.

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

acbramley → created an issue.

Can and should we cache a revision list?

I think in a real world scenario we'd almost never hit the bug that this test is hitting. To reproduce it via a browser I had to:

1. Disable the admin theme setting for node.settings
2. Disable the breadcrumb and primary tab blocks (these add the node cache tag)

Without doing this the node cache tag gets added to the page and when editing a node via the UI to create a new revision, the revisions page is purged.

I think it's perfectly reasonable to have a cacheable revisions page, especially since the reason why it isn't is because of a bugged call to addCacheableDependency (which I thought we deprecated in #3232018: Trigger a deprecation when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object → but that didn't cover the renderer...)

Reversing the terminology and adding the summary line back as well as changing the "developer" wording. Still need to add examples but I'm not sure where they should live (probably under all of the text?) and how detailed we want to be.

acbramley → changed the visibility of the branch 3490787-remove-exception-when to hidden.

Here's the crucial difference:

11.x has node.settings:use_admin_theme defaulting to true https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/...

11.1.x has it defaulting to false https://git.drupalcode.org/project/drupal/-/blob/11.1.x/core/modules/nod...

DPC has a response policy to blanket deny on admin pages via Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes

This default setting was changed in 📌 Consider using the administration theme when editing or creating content by default Active , does that just need to be committed to 11.1.x?

It looks like the difference is to do with some other unrelated cache metadata making this cacheable on 11.1.x when it's not on 11.x (in the context of NodeRevisionsAllTest)

11.x cache headers:

  'Cache-Control' => 'must-revalidate, no-cache, private',
  'X-Drupal-Dynamic-Cache' => 'UNCACHEABLE (response policy)',
  'X-Drupal-Cache-Tags' => 'http_response rendered',
  'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args user.permissions user.roles:authenticated',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',

11.1.x cache headers:

  'Cache-Control' => 'must-revalidate, no-cache, private',
  'X-Drupal-Dynamic-Cache' => 'HIT',
  'X-Drupal-Cache-Tags' => 'http_response rendered',
  'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args.pagers:0 url.query_args:_wrapper_format user.permissions user.roles:authenticated',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',

Removed the jQuery selector, I'm not sure if this is testable in phpunit.

The original feature was added in 🐛 Double click prevention on form submission Fixed in 2014 without tests but that could have been before we had extensive test coverage.

@mradcliffe I agree, but the MR here was adding the library's code to the module which I don't want to support. We should find an alternative.

I think we can close this one now that the original is reverted and test both branches over there?

I've gone through the MR quite thoroughly and generally +1 this overall approach although it is quite complicated and may be hard for some devs to grok, especially the compiler pass stuff (queue many "why isn't this working?!")

My main concerns are:
1. The administer_blocks_permission_test module, although very minor it just doesn't feel right to add a duplicate permission, this is a big nit picky though
2. navigation_variant_test - now tests that just needed 1 block get all the additional elements rendered. Again this may be minor. I haven't ever looked into DisplayVariant plugins before though so thanks for the learning :)

The bug is that the access checks are invalid and functionally don't do anything, the issue summary is up to date with this information and the original bug report is there as well.

What bug is it causing?

See https://www.drupal.org/project/diff/issues/3498207#comment-16113370 🐛 Required library has code deprecation Active

Running composer update drupal/diff -W should be enough.

Since the caxy library is a dependency of a dependency (mkalkbrenner/php-htmldiff-advanced) there's nothing else to do here.

You can disable layout plugins via the module settings page. Disabling all but 1 will not allow users to change to any other layout plugin.

Done

For example Layout Builder renders reusable Block Content with contextual links on the view entity route just fine.

Layout builder does this via a block plugin, no?

@tr this issue is asking for a plan for the release, can we at least get the issues listed here that need to be fixed? Then this can become a meta and we can work through them together. Right now it is not clear what needs to be done.

No, this is just removing invalid code.

@alexpott I think that's a much larger task, we'd need to:

1. Add a new formatter
2. Update views
3. Write an upgrade path for any view that has a revision_log field to change the formatter.

Step 3 seems like it could be extremely disruptive.

The revision log is a plain text field, it should not contain HTML.

I'll need some more information, what commands have you run and why are you unable to upgrade.

smustgrave → credited acbramley → .

I'm not fully understanding what the IS is asking, but we cannot expose inline blocks via a UI outside of layout builder. The layout entity must reference the revision id of the inline block so editing the block (and creating a new revision) outside of the layout entity will break things badly.

More information at https://www.drupal.org/project/drupal/issues/3075308#comment-13322570 🐛 Inline blocks shouldn't be editable via the normal block content UI Active

The Recipes initiative added the ability to import content via yml files

It is not very well documented but the class is Drupal\Core\DefaultContent\Importer and some information at https://git.drupalcode.org/project/distributions_recipes/-/blob/1.0.x/do...

Block content won't be implementing it's own thing for this.

The blocker went in but I don't think we should try combining the screens, there's a push to decouple block_content from block at the moment so we don't want to tie them together any more (see 📌 Why block_content depends on block? Active )

There is also ✨ Link block to content_block Active for adding a link so maybe we close this one in favour of that?

Here's the explain on the query from loading the view and clicking the tablesort.

MySQL [local]> explain SELECT node_field_data.nid AS nid, users_field_data_node_field_data.uid AS users_field_data_node_field_data_uid
    -> FROM
    -> node_field_data node_field_data
    -> INNER JOIN users_field_data users_field_data_node_field_data ON node_field_data.uid = users_field_data_node_field_data.uid
    -> WHERE (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1)
    -> ORDER BY users_field_data_node_field_data.name ASC
    -> LIMIT 50 OFFSET 0
    -> ;
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
| id | select_type | table                            | partitions | type | possible_keys                                | key     | key_len | ref                       | rows | filtered | Extra                           |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
|  1 | SIMPLE      | node_field_data                  | NULL       | ALL  | node_field__uid__target_id                   | NULL    | NULL    | NULL                      |    1 |   100.00 | Using temporary; Using filesort |
|  1 | SIMPLE      | users_field_data_node_field_data | NULL       | ref  | PRIMARY,user__id__default_langcode__langcode | PRIMARY | 4       | local.node_field_data.uid |    1 |   100.00 | NULL                            |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
2 rows in set, 1 warning (0.000 sec)

So yeah we're using temp tables.

Setting to PMNMI for the decision on whether we want to do this. Personally I'm happy to close.

actually this is probably still NW as we'd need changes to the test to cover the redirect error.

Merged in 11.x, the test was reverted to what was committed in the other issue and the only difference in NodeAccessControlHandler was an additional comment which I've kept.

This is now green

The last outstanding thread is https://git.drupalcode.org/project/drupal/-/merge_requests/10036#note_40...

Not sure how to handle BC for this.

acbramley → created an issue.

Added https://www.drupal.org/node/3524296 → and linked to it from the other CR.

I don't think we need a CR for the MediaSourceInterface::METADATA_ATTRIBUTE_LINK_TARGET, happy to be told otherwise though :)

Will try to get this green today

From the steps to reproduce, this sounds like an issue in BEF?

acbramley → created an issue. See original summary → .

More coupling in BlockContentForm https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...

The BlockContentViewBuilder is also tightly coupled to Block module.

And more info in 🐛 Block Content entities have no Contextual links when rendered outside of Block config entity Closed: won't fix

We also have FilterNodeAccessTest which tests this filter, it obviously passes now. Changing from AND to 0 doesn't break it, but we'd need test coverage for the bug here.

There's no code here to add tests for.

Drupal 10.2 is also EOL, is this reproducible on Drupal 11?

I added the condition to the content view on standard install and logged in as a non admin user and can't reproduce an error. It does look incorrect though to pass AND there but the query does seem to work, at least without any grants.

Forgot to set back to rtbc post rebase.

FYI I haven't changed anything to do with DI except in NodeModuleHooks where both hooks need the module handler.

I'm not sure how we should handle it, we've seen impacts to performance tests in previous changes and I've also seen how they can break Kernel tests where a test doesn't install the module that implements the service being injected, since the test doesn't hit the hook it's not necessary.

Maybe we should defer the DI discussion to another issue and get the organisation of hooks in first since it's going to be quite disruptive already.

Split by api.php as per #29

acbramley → changed the visibility of the branch 3511357-remove-nodehooks1-2 to hidden.

acbramley → changed the visibility of the branch 3511357-remove-nodehooks1 to hidden.

Got this green, it would be great for people to test this on 11.x with the latest changes in the MR.

God help the next person that has to deal with SmartDefaultSettingsTest 💀

acbramley → created an issue.

I accidentally rebased onto the closed branch so I've toggled MRs since the latest one had other issues.

Please, if you want to contribute to this issue do not upload more patches or open more MRs, instead get push access via the button under the issue summary and contribute directly to this MR

https://git.drupalcode.org/project/drupal/-/merge_requests/11100

acbramley → changed the visibility of the branch 3274635-upstream-use-ckeditor-11.x-2 to hidden.

acbramley → changed the visibility of the branch 3274635-upstream-use-ckeditor-11.x to active.

This is great, I've done the following:

1. Gone through the MR and closed all threads after confirming they were resolved or (in the case of the helpful comments) explained what was going on well enough.
2. Reviewed the code
3. Rebased the MR to ensure tests pass against latest HEAD.
4. Manually tested the changes locally with xdebug ensuring the closure works as expected.

This is ready to go!

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

And I disagree that it up to the developer to decide

I think "developer" is used very broadly here to include reviewers/committers (if you're reviewing code or committing it you're probably a developer too?) but we can use more generic language.

One thing that can be done to help here is examples of when the docs must be added and when the docs can be omitted.

That is going to drastically increase the length of the description though?

Can we add consistency so that the 'return' section has separate line like "All @param tags on a function can be dropped, if:" ?

There is only ever a single return tag so it doesn't make sense IMO.

Why is the summary line removed? That seems out of scope.

I removed it to shorten the overall section because it's implied by the other information.

Patches in #119 and #120 seem to be missing something, again it's hard to figure out what since no interdiffs have been provided. When applied to 10.3.x I'm unable to save the editor config screen as it complains about the properties key in config schema.

Going all the way back to #86 is working on 10.3.

🐛 Action buttons unresponsive when coming back from node preview Active was marked as a duplicate of this.

When testing that issue I also found some weird behaviour (I didn't realise this was core's JS at the time) where after you hit Back in the browser you can eventually submit the form if you spam the save button a few times.

@ulethjay great find, thanks!

quietone → credited acbramley → .

This is at the top of the postponed list, was a duplicate ever found?

The title doesn't really seem to match the IS or patch either, wondering what we should do here.

It's been another few months without any steps to reproduce so closing this one out.

If you are able to provide steps to reproduce, feel free to re-open this.

Addressed feedback, service names are personal preference I guess? Personally not a huge fan of a class name service and interface name alias.

Found a nice way to do #16 - there's a _block_ui_hidden key on the definition that BlockLibraryController uses to filter out definitions when adding a block. This doesn't affect existing blocks that have already been placed. Works perfectly for what we need here!

Improved the solution substantially with:

1. Config subscriber as requesting by @alexpott to clear theme settings caches when core.extension:theme changes
2. Memory cache service following recommendations in 🌱 [policy] Standardize how we implement in-memory caches Needs work

I've tried passing the display's cache tags into the set() call so, in theory, it should be invalidated automatically when the display is saved but that doesn't seem to be the case. Must be missing something?

Tested this manually on HEAD and was unable to reproduce the access denied issue.

Much nicer solution, thanks for the links. I didn't see any decisions on 🌱 [policy] Standardize how we implement in-memory caches Needs work with how these memory cache services should be setup wrt. service id names or how specific/generic they should be but I've loosely tried to follow what other things are doing in core already with cache.asset_memory and system.module_admin_links_memory_cache

Awesome! Have added that and removed the classes. Tested manually and it works well.