🇦🇺Australia @acbramley

It has been 5 months since #82, wanted to give this one last bump before closing it.

Closing this for now, if you feel strongly that this should be implemented please feel free to reopen.

Now expanded to the generic revision UI as well.

Same failure as #41 with test-only changes "0 elements matching css "table tbody tr" found on the page, but should be 50."

Added test coverage for the issue, here's what the revisions page looks like on HEAD in that test case:

revisiting this with fresh eyes I realised we needed to filter by revision_translation_affected, adding that means we are back to where we were in terms of revision displays (tests pass with no changes). Now we need test coverage for what we're fixing (e.g broken pagers with a lot of unaffected revisions)

Took me a while to figure out what was going on, the DataProvider was pointing to the wrong method!

@mrshowerman thanks very much for the link, confirming that the fix from that issue (https://git.drupalcode.org/project/drupal/-/merge_requests/12124) does fix this issue. Agree that it should be closed as a dupe.

Confirming this fixes the issue for me, I found this after upgrading to 11.2 and Gin 5. Back to NR.

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

It would be possible to leave the library empty

I tried that and you can't, you get an error about an incomplete library

I've tested across Gin, Claro, and Demo Umami (with editing in admin/fe theme) and these classes aren't used in any of them. I think we can even remove the node-edit-form.html.twig from umami as well as that is not used at all.

To make things even stranger:

node.module.css is in 2 different libraries - node/drupal.node and node/form
Both libraries are only attached in a single place in core - NodeForm.php
Claro removes the node.module.css file from both libraries via libraries-override
stable9 replaces node.module.css from both libraries with its own node.module.css file

I think in this issue we should also remove the node/form library entirely.

Looking at node.module.css we have styles for:

1. .layout-region this only has one rule box-sizing: border-box; which Claro also has in its form-two-columns.css. This looks pretty safe to remove
2. .layout-region-node-main and .layout-region-node-footer and .layout-region-node-secondary These classes only appear in 3 places that aren't CSS files. Those are node-edit-form.html.twig in the demo_umami theme, the starterkit_theme, and in gin itself. Gin also has a lot of CSS rules for these same classes.

Funnily enough, none of these classes (apart from layout-region) are on the node form after installing standard profile both with and without the "Use the administration theme when editing or creating content" setting.

TLDR; these are looking quite safe to remove. I will need to test across a few of these areas as well as with the vertical toolbar enabled as there are some special rules for that too.

Also need an IS update here, I'm guessing from the existing patches this is just referring to the styles in node.module.css

Here's the duplicate 🐛 Revisions are not visible for some nodes Needs review

This is a duplicate of 🐛 Node revision UI will sometimes show no revisions on a page RTBC and 🐛 Revisions are not visible for some nodes Needs review

They should actually be removed, postponing this on 📌 Consider showing all revisions on the revision overview Active although I think we have other duplicate issues in the queue.

That does reflect the database but will it make sense to the editor?

I agree it's quite a large change, but if you look at all the related issues and #27 you'll see the current situation is really not ideal. I'd rather see more detail than not be able to see what's actually going on.

I think the reason why we see new revisions for the default language is because revisions are created for the default language as well?

Thanks for working on this, it should be straightforward to add test coverage for this to the existing tests

Nightwatch tests are extremely inefficient and are likely being phased out eventually (see 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active ).

The test here can be rewritten as a functional test that just checks for the label and uses a test view that's already defined in yaml.

We found issues with this hunk of code after deploying to production:

    if (\Drupal::moduleHandler()->moduleExists('path')) {
      $route = \Drupal::service('path.validator')->getUrlIfValid($source_path);
      $allow_alias = $this->config->get('allow_from_alias') ?? 0;
      $skip_alias = !$allow_alias;
      if ($route && $skip_alias) {
        // This URL path has a valid internal route and the Redirect settings
        // are not configured to allow redirects from aliases. Do not redirect.
        // See https://www.drupal.org/project/redirect/issues/2879648
        return [];
      }
    }

This was breaking preview_link functionality through some weird interactions with getUrlIfValid.

This was added in https://git.drupalcode.org/project/redirect/-/merge_requests/109/diffs?c...

@mark_fullmer do you recall exactly what this was achieving? From my testing, we don't need this check at all. With some better handling in the path processor we can simply not check for the alias when the config isn't set (see latest commit).

I also fixed an assertion in one of the new tests.

I've reverted the documentation page.

The tricky part here is that setComponent is used both for adding fields to the layout when Layout builder is initially enabled, and when adding new fields.

Both in the case of the old feature flag and removing the code outright, we get an empty layout when enabling layout builder. IMO this is a good thing, but just wanted to point that out if it wasn't obvious before.

We could retain that functionality and just move the code out of setComponent and into a new method called by the preSave which is where the initial addition of the fields when layout builder is enabled happens.

acbramley → changed the visibility of the branch 3115759-prevent-auto-adding-new to hidden.

acbramley → changed the visibility of the branch 3115759-prevent-auto-adding-new to active.

What's your timeline on getting 2.x stable? Why don't we do a 3.x branch that is 8.x-1.x that drops unsupported versions of core/php. Once 2.x is stable that could either continue on a 2.x line or become 4.x.

I understand where you're coming from, but even Drupal 10 requires at least PHP 8.1 → . Postponing 8.4 fixes on the off chance someone is using an ancient version of PHP feels backwards to me, especially since we're soft-blocked on a new major given the architectural changes in 2.x that is not production ready.

acbramley → changed the visibility of the branch 3496146-deprecations-php-8.4 to hidden.

Green!

Might be good to get 🐛 Resolve test failures from once() called statically Active in first to get a green pipeline for this.

PHP 7.4 has been EOL for almost 3 years, why would we need to support 7.0?

This is NW as there are more issues that need fixing

acbramley → changed the visibility of the branch 3543251-fix-phpunit11-failures to hidden.

Needs porting to 8.x-1.x

Searched 3 different keywords and somehow didn't find the existing issue

acbramley → created an issue.

@logosur $node->getOwner()?->getAccountName() - the ?-> in that is a null safe operator, meaning if getOwner returns NULL it will not call getAccountName.

This content is specifically to show the node's owner/author, we don't need any more logic to show the current user for new nodes as this is set on save automatically. If there's something else you'd like to change then please create a new issue as this one is closed.

Closing this out as it's been >3 months since this was moved to PMNMI.

If you're still experiencing these issues, feel free to reopen this issue with more details on how to reproduce this. Thanks!

Good call, swapped to a kernel test.

any reason for us not to upload a patch from the MR diff here for folks to use

The main reason is it confuses contributors, if someone sees a patch they may start contributing fixes via patches again rather than the MR. It then becomes hard to track which changes have been made and whether they have been pushed to the MR or not. It's also very frowned upon in the core queue as it can confuse the needs-review-bot which will put an issue into needs work if the patch no longer applies even if there is an MR, therefore I usually discourage it.

I have used the MR diff as a patch against 1.12 and #105 reports it is working for them as well.

FYI it's not a good idea to use .diff MR urls directly in your composer patch files. This is a security risk as anyone can gain push access and push code to it which could cause all sorts of issues.

Instead, download the diff and store it locally and reference the local patch file instead.

acbramley → created an issue.

This looks like a duplicate of 📌 Fix the issues reported by phpcs Postponed and 🐛 Fix PHP 8.4 implicit nullable deprecation Needs review

This is a duplicate of 🐛 Fix PHP 8.4 implicit nullable deprecation Needs review

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

@amateescu would it be possible to get a new stable release with these fixes?

Ran with the idea in #3 a bit, I'm not sure on the implications of allowing page_top and page_bottom in HtmlResponseAttachmentsProcessor. I tried playing around with unsetting them in buildPageTopAndBottom with a pass by reference on $main_content but that didn't work.

Setting to NR to gauge how viable this would be.

FYI we also have 📌 Get rid of node_page_top() Active which would make this moot.

Squashed and rebased.

Can't wait for those property hooks, this is looking good now

Thanks @farse indeed this was fixed in 🐛 TypeError: key(): Argument #1 ($array) must be of type array, null given in key() (line 424 of modules\contrib\entity_browser\src\Plugin\Field\FieldWidget\EntityReferenceBrowserWidget.php) RTBC

This was a duplicate of 🐛 Warning: key() expects parameter 1 to be array, null given in Drupal\entity_browser\Plugin\Field\FieldWidget\EntityReferenceBrowserWidget::processEntityBrowser() RTBC which was open since 2017.

Sounds like we need to postpone this issue then?

Found a few more easy replacements

Checking other spots we can use this actually.

Beauty, thanks!

This is green now and we have the config option to opt-in to this. Would this make it more appealing to commit?

@emilorol you need to apply a patch as the fix is not yet in a release.

Similar to 🐛 Validation issue on adding url redirect Needs review the merge conflict resolution here completely broke this MR. I've reverted back to the previous commit and re-merged. The RedirectRepository conflicts were quite hard to resolve but the alias test at least seems to be passing.

@rajab natshah please be careful when resolving conflicts that you don't break the changes. You had reverted large chunks of the MR here and left it in a broken state. It's usually best to just do a single merge commit when resolving conflicts, or try to rebase the branch.

It then becomes very hard for other contributors because we must then painstakingly go through each file and compare changes before the conflict resolution to see what was been broken.

acbramley → created an issue.

acbramley → changed the visibility of the branch 8.x-1.x to hidden.

Restoring IS

I've removed the event subscriber and added the cache tag, IIUC that's all that is needed

Added 📌 [PP-1] Use EntityBundleInfo::getBundleLabels in MediaEmbed::SettingsForm and elsewhere Active

acbramley → created an issue.

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

The problem is that would be an extraordinary amount of work. It's not just block using those variable names, there's 56 twig templates alone with this variable in them in core. This spans blocks, comments, fields, media, nodes, search results, taxonomy terms, views, and more. The effort to deprecate title_prefix and title_suffix with BC in mind would be a huge amount of code for what IMO is very little gain.

@smustgrave there is no upgrade path and I wouldn't want to do one (see #26)

Created 📌 Handle language deletion generically for any entity type Active

acbramley → created an issue.

Thanks so much @mark_fullmer

bad bot.

Rebased cleanly via the UI for me