🇦🇺Australia @fenstrat

This still applies to the 2.2.x branch.

MR 33 should be the correct fix, i.e. just a missed update from entity_type to entity_type_id.

I'll circle back and update all the tests where this is affected after 📌 Improve test coverage and add back "Allow voting" display option Needs review lands.

fenstrat → created an issue.

#21 looks good. However for the test, rather than just tack the asserts onto an existing test, I think it'd make more sense to split it off into it's own test method? Also, the cancel button should work without JS right, so does this need to be an Ajax/WebDriverTestBase test (i.e. could it be in Functional\FivestarTest instead?

Would also be nice to see this go green once the expanded test coverage over in 📌 Improve test coverage and add back "Allow voting" display option Needs review lands.

I'm not sure that any of these approaches are actually correct.

Could someone please test the fixes in 📌 Improve test coverage and add back "Allow voting" display option Needs review to see if it solves their issues here? Specifically it fixes how allow_ownvote works (it wasn't working at all when fivestar was used when viewing the entity). With #3447756 applied I cannot replicate the unneeded vote as per the "How to reproduce" steps above.

If issues persist, please outline the steps to reproduce.

Thanks for working on this. It'd be great to get even a super basic test for these token in please, as @TR mentioned in #7.

Bumping back to NR for the tests, but also to hopefully get some other eyes on it.

@heddn and @TR thanks for all your work here!

I've created a follow up to further improve test coverage: 📌 Improve test coverage and add back "Allow voting" display option Needs review as a lot of the tests from #26 didn't end up making it in here. 3447756 also adds back the allow_vote option which didn't make it in here either.

Sorry for the radio silence in all the follow ups post #26 - the project I needed this on stalled for a long time. Thankfully it's ramped back up so I should be able to dedicate a bit of time to helping polish up fivestar.

MR 32 does the following:

1. Improves the test coverage, much of this was based on #3131954-26: Port tests to 8.x-1.x → but which were never included as part of that issue.
2. Test reworking includes updating FivestarTest::testViewerNonRating() which was testing a non existent disabled option which was never ported from Drupal 7. This is what I've added as allow_vote given that disabled is a pretty overloaded term.
3. Also worth noting that FivestarTest::testViewerNonRating() was added to in #3190556: Illegal choice 0 in field_fivestar element for fivestar widget → , however that additionis now covered elsewhere in FivestarTest. Aside: At a glance I can't say I understand what was achieved in #3190556 ¯\_(ツ)_/¯
4. Note I've pulled in some of the test changes made to FivestarTestTrait in #3201584-22: StarsFormatter - Add back user, smart and dual option for star display and text → as they were also needed here.

Would be nice to get this test coverage reviewed and in, at which point I'm happy to circle through other open issues and update them as needed.

fenstrat → created an issue.

fenstrat → created an issue.

In MR30 I've re-rolled the approach from #6 and also updated the tests.

1. Changes to constructor property promotion in VoteResultManager
2. Removes FivestarAjaxTest::createFivestarFieldTwo() as it was unneeded duplication, instead FivestarTestTrait::createFivestarField() has been updated to support the required options when creating fields for testing.
3. Fixes an issue in `Drupal\fivestar\Element\Fivestar` that was introduced in 📌 Fivestar Element drupal-check issues. Fixed where the 'smart' formatter wasn't working as expected. Yay for tests catching this.

Thanks @danrod, confirming this works for D7, so it's RTBC.

It's also the same approach that was committed and working for D10+ in 🐛 No longer protecting Fixed .

@Steven Jones yes I did, everything mostly worked from memory. Needed a couple additional patches, some of which you've committed, thanks!

More to the point though Aegir 3 seems pretty much at a dead end, as you well know. Especially when I was looking at this ~ 6 months ago. As we needed Drupal 10 support we've had to migrate off Aegir. Thankfully replacing its platform migration (the essential bit of Aegir for us) with custom code that did what we needed was relatively straight forward.

Here's an updated patch bumping the max supported to v15.0 because v12.0 is EOL a few days (8/2/2024). I've gone with v15 as it is likely to have an EOL in early 2025 (still to be confirmed) which is when Drupal 7 will also be EOL, so this is likely the last update this would ever need as all sites should be updating to Drupal 10+ by then.

Tested both registration and login and it's working for that.

Confirming #6 is RTBC for the 1.x branch of group.

Here's a patch for D10, leaving as RTBC.

Attached is an update of #5 which removes the rector comments as all the changes it made look correct.

Here's an updated patch bumping the max supported to v12.0 because v11.0 (which 6.0 appeared to get automatically bumped up to?) was EOL a couple days ago.

Also tested both registration and login and it's working for that.

On the 7.x-3.x branch it looks like Drupal 10 support is going to be tricky, and quite likely impossible. https://github.com/omega8cc/boa/issues/1678#issuecomment-1657210645 is a decent summary of the issues. Basically Aegir 3.x will only ever be able to use Drush 8, and it's essentially impossible to make Drush 8 work with Drupal 10.

Given Drupal 9's EOL in Nov 2023, and therefore requires sites to update to Drupal 10, it looks like Aegir 3.x could essentially EOL for anyone hosting Drupal 10 sites.

I'm unsure of what the status of the 7.x-4.x branch is.

The 5.x branch looks to be making great progress. However as multisites basically won't be supported in 5.x (which generally makes sense) our usecase isn't going to be covered. So for us at least this looks like the end of the Aegir road.

Confirming #3 also fixed the issue for me, with PHP 8.2.

fenstrat → created an issue.

fenstrat → created an issue.

fenstrat → created an issue.

This LGTM.

Attached adds one more case where I was seeing Trying to access array offset on value of type null in omega_pager(). Leaving as RTBC.

#2 looks like the correct approach and fixes the Deprecated function notice.

Attached is a simple reroll that fixes tabs with space.

fenstrat → created an issue.

Same patch, but combined with #2998663-2: Error "Warning: A non-numeric value encountered in alpha_theme_container->regions() " on php 7.1 → as when that is applied this one doesn't apply, think it's due to the missing blank line at the end of the file.

fenstrat → created an issue.

fenstrat → created an issue.

fenstrat → created an issue.

The patch in #3 fixes the issue for us.

It could be related to MySQL version as we first hit this with MySQL 8 (bumped from 5)

Actually a couple more nested ternaries needed adjusting. These things are a nightmare.

The cause of this is the templates in Provision/Config/Drupal/provision_drupal_settings_*.tpl.php which writes out the settings.php for each site. Attached updates those to use '' rather than null.

After applying this to provision all effected sites need revalidating in Aegir.

This is a duplicate of 🐛 php 8.2 deprecations warnings Fixed which pre-dates this issue.

@bgm thanks very much for the details in #7. Using that I've successfully migrated from /var/aegir/hostmaster-7.x-3.192 to /var/aegir/hostmaster

Attached is the patch I used for hostmaster that bumps the contrib modules up to their latest stable versions. Note that building hostmaster with drush make already bumps core to the latest stable (7.98).

To apply the patch, before running drush make edit /var/aegir/.drush/provision/aegir.make to get it to apply the patch:

diff --git a/aegir.make b/aegir.make
index d1357cc8..75d2c18f 100644
--- a/aegir.make
+++ b/aegir.make
@@ -7,4 +7,5 @@ projects[hostmaster][type] = "profile"
 projects[hostmaster][download][type] = "git"
 projects[hostmaster][download][url] = "http://git.drupal.org/project/hostmaster.git"
 projects[hostmaster][download][branch] = "7.x-3.x"
+projects[hostmaster][patch][3317637] = "3317637-21.patch"

Running drush make /var/aegir/.drush/provision/aegir.make /var/aegir/hostmaster will then use the latest contrib modules.

Next step for me is to bump from PHP 7.4 to PHP 8.2. I'll report back if there's any issues.

This is a duplicate of 🐛 Deprecated: Unparenthesized `a ? b : c ? d : e` is deprecated. Use either `(a ? b : c) ? d : e` or `a ? b : (c ? d : e) Needs review which already has a patch.

Looks like the correct approach. #2 fixes static analysis:

The left-associativity of the ternary operator has been deprecated in PHP 7.4 and removed in PHP 8.0. Multiple consecutive ternaries detected. Use parenthesis to clarify the order in which the operations should be executed

#2 looks correct.

Attaching an updated patch that address 2 more PHP 7.0+ issues:

Indirect access to variables, properties and methods will be evaluated strictly in left-to-right order since PHP 7.0. Use curly braces to remove ambiguity.

Here's a patch version of the MR in #17.

The patch in #19 doesn't remove the original debug_backtrace() call.

Also confirming that #25 fixes the issues I'm seeing which are the same as from #7.

Would be nice to get this into a new release.

@mstrelan just pointed out that Symfony also use UpperCamelCase https://symfony.com/doc/current/reference/forms/types/enum.html

Another +1 for UpperCamelCase for Enum cases. PHP docs also use it: https://www.php.net/manual/en/language.enumerations.basics.php

This sounds like a duplicate of 🐛 [upstream] [GHS] CKEditor 5 removes s that wrap HTML elements not natively supported by CKEditor 5 Fixed , however it seems a bit different. That's because in 3349893 it's about an inline element (i.e. an <a> tag) wrapping a block element (i.e. a <div> tag). Whereas here the wrapping h2 is a block element. Not sure if that affects this issue here, but thought it was worth noting.

@RedNeko thanks for your work on the CKEditor4To5Upgrade Plugin! Confirming the hook_ckeditor4to5upgrade_plugin_info_alter is the correct approach. It looks good to me.

@neclimdul re linking the embedded entity with a wrapping div, hmm I don't think that'll worth either. See the markup that was tried 🐛 [upstream] [GHS] CKEditor 5 removes s that wrap HTML elements not natively supported by CKEditor 5 Fixed which seems to use a wrapping div.

Noting here that this blocks entities embedded via entity_embed from being linked to other content. That was possible in CKE4 thanks to ✨ Image entities/fields embedded using Entity Embed cannot be linked in CKEditor Fixed

I'd missed the fact that embedded entities cannot be linked. This was possible in CKE4 thanks to ✨ Image entities/fields embedded using Entity Embed cannot be linked in CKEditor Fixed which we'd used to allow linkit links for embedded entities.

Unfortunately this is block upstream in CKEditor 5 itself. This is being tracked. 🐛 [upstream] [GHS] CKEditor 5 removes s that wrap HTML elements not natively supported by CKEditor 5 Fixed and is listed as issue 4. in the tracking issue 🌱 [meta] [upstream] Prioritized CKEditor 5 upstream blockers Active .

Also confirming after initial testing the MR here is working well.

A remaining task is listed as adding a CKEditor4To5Upgrade Plugin, but I'm thinking there'll be a bit to that as each site will have a customised list of cke4_buttons that need converting? The plugin would need to load each embed.button and add them as cke4_buttons to be converted.

Whups, didn't mean to change the issue summary.

Initial testing of the MR with the changes from #45 seems to be working well.

It's not clear why we're keeping the dependency on fakeobjects. But as that is staying (for now at least) then in order to uninstall ckeditor (i.e. version 4 from contrib) you'll need 📌 Remove dependency on ckeditor as fakeobjects can also be used with ckeditor5 RTBC .

Attached implements approach number 2.

fenstrat → created an issue.

@alfattal 🐛 csv_serialization not compatible with Symfony D10 Closed: duplicate was marked as a duplicate of #3280951-15: Drupal 10 compatibility → , so it's already fixed. Just use the 3.0.0 release, it's in there.

Just confirming that in our use case our media revisions have a couple dozen revisions at most. Most of them have far less. So probably explains why we haven't seen any performance issues here.

The approach of deprecating the age argument makes sense. Happy to test it out in our use case.

@Berdir hmm interesting, I tried that so:

diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php
index 27b01384be..2b0585ca87 100644
--- a/core/modules/file/src/FileAccessControlHandler.php
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -38,11 +38,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
               $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)
                 ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
               if ($referencing_entity instanceof RevisionableInterface && !$referencing_entity->isDefaultRevision()) {
-                // @todo The 'view all revisions' permission is provided only
-                //   by the node entity type. Use the generic permission name in
-                //   https://www.drupal.org/project/drupal/issues/2809177.
-                $revision_access = AccessResult::allowedIf($account->hasPermission('view all revisions'))
-                  ->orIf($entity->access('view revision', $account, TRUE));
+                $revision_access = $entity->access('view revision', $account, TRUE);
                 $entity_and_field_access = $entity_and_field_access->andIf($revision_access);
               }
               if ($entity_and_field_access->isAllowed()) {

However then this assert fails (locally, when it was passing before):

$this->assertTrue($access->isAllowed(), 'Confirmed that a file referenced in an old node revision is accessible if user has role view all revisions.');

So that looks like it contradicts what you've said?

Noticed a line wrap issue in the test, attached fixed that.

Good catch with #130. Attached updates that.

However $entity->access('view all revisions') still fails, as it seems that is only provided by node module? So I've kept it as $account->hasPermission('view all revisions') and left the @todo note. I have added the OR with $entity->access('view revision').

This is ready to go, fantastic work everyone.

I reviewed this from the perspective of a site that's making use of https://www.drupal.org/project/block_content_permissions → - good news is that that module should be obsolete now once this issue gets in.

The one permission block_content_permissions defines I wasn't sure on is view restricted block content "View full list on the Block Content overview page ignoring the create/update/delete filter.". However the new access block library introduced here and assigned to the block_content view covers that nicely.

I also think that using the user facing "custom block" doesn't feel right, however agreed that it is a good compromise. And it'll be fixed in the follow up 📌 Rename Custom Block terminology in the admin UI Fixed .

Also, fixed a minor CS issue with the @todo comment format.

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

Nicely done, can't fault it :+1:

This is ready to go.

I manually tested this with the D9.5 > D10.0 update mentioned in #14 and confirm that this fixes the issue.

Agreed this would be good to backport to 10.0.x

Thank you kind sir!

So in our case the admin context check in isWebformAdminRoute() is picking up the admin route (which oddly goes against what #2 is suggesting - that was our issue in the first place, \Drupal::service('router.admin_context')->isAdminRoute() === FALSE). Though @acbramley noted he did see 2 requests when debugging through getCurrentRequest().
So then because it is being seen as an admin route that means it then falls through to the (preg_match('/^(webform\.|^entity\.([^.]+\.)?webform)/', $route_name)) ? TRUE : FALSE; check which picks correctly picks up the admin route.

As that last check is done on route_name, and that seems to be more reliable in hook_css_alter, then this seems fine.

Also, reusing \Drupal::service('webform.request')->isWebformAdminRoute() and deferring to existing code seems like a sane approach here anyway.

Here's the WebformRequest::isWebformAdminRoute() approach. It fixes the issue in our case. But it's not convincing as the correct fix™ because isWebformAdminRoute()is still accessing the admin context.

Confirmed simply removing views_data_export_auto_download.js works.

As there's no Drupal 10 specific branch of views_data_export, this will need to be tested on Drupal 9, but it should also work there.

Good point about \Stringable being php8.0+

In the MR I've bumped php to 8.1+ (as D10 already requires that) and the core_version_requirement: ^10. So this can go into a new 3.x branch.

As for coverage, yep, we are indeed missing that. There's no existing functional that's doing any checking (or rechecking which is where we hit this fatal), so it'll need that the crawler service swapped out as a minimum. Bit to it, happy to take a look, but might make sense in a follow up?

This time with an actual patch!

Not sure what's going on that with test run failure. Will look again later, he's the change in patch format for now.

fenstrat → created an issue.

Hi @TR, thanks for your work on this.

I have to say it's a little frustrating given that you're inching your way back to the passing tests I had in #26. When you've reached some finishing point I'll be happy to review it and compare it to what I had, and also run my project specific tests which are built on top of #26.

I think the test failure here is unrelated, see #3328181-14: Deprecate "Only log entities for published content" setting and refactor hooks. → for the fix.

Confirming that #14 fixes the test failures in GatsbyEntityLoggerTest. This is when running the tests locally.

Odd that the CI test runs seem to be coming back with Build Successful, but then not actually sending the results back.

Also, concerning that the commit in #11 happened without tests passing?

I don't think #13 is the correct approach. Prior to D10 the default was accessCheck(TRUE) on any query which didn't specify otherwise, so I think we should be defaulting to TRUE here.

Thanks @DamienMcKenna, I'd not appreciated that was the plan. Will continue in 📌 Drupal 10 compatibility issues Fixed .

Marvellous, thanks Lee!

Good catch with the updates hooks, have nuked them too and added the removed hooks.

Switched to an MR as the patch didn't like the removed .gz's (could have switched to --binary but just went with an MR)

Just the updates tests are failing, on field storage definition changes and friends. As we're going to D10 and all of these update tests are D8 > D9 related I think it's time to remove these, just as core does.

Bot missed some other version constraints.

Turns out 3.x of build_hooks is where this work should go (as there's no real point in continuing 2.x), will move this into 📌 Automated Drupal 10 compatibility fixes Fixed .

Lets try that again, #2 was based on 8.x-2.6.

This is based on 8.x-2.x-dev.

Make sense, thanks @dpi!

I think this one might need a follow up, opened 📌 Access checks for denying current revision revert/delete need work Active for that.

My understanding from the Slack thread is that the site where this was reported applied the patch from ✨ Add "edit block $type" permissions Fixed to a 10.0 site, and that applying the +1/-1 patch from the follow-up issue #3315042 was also enough to fix the performance issue.

Yeah sorry I wasn't clear there, that is indeed the case. What I was commenting on here was when we reverted #3315042: Remaining tasks for "edit block $type" permissions → , i.e. we were using EntityPermissionsRouteProviderWithCheck rather than EntityPermissionsRouteProvider, then when the approach in the MR in this issue was applied that our performance issues also went away.

fenstrat → created an issue.

Forgot to mention, I came across this while making similar changes to media_revisions_ui in 📌 Drupal 10 compatibility fixes Fixed .

The tests there are what made me dig into find out what was going on. Adding tests here would probably add a lot more confidence around these changes.

fenstrat → created an issue.

This ended up being done as part of 📌 Drupal 10 compatibility fixes Fixed as the deprecated MediaRevisionAccessCheck was removed in D10.

Thanks!

As this requires Drupal 10.1 I think it makes sense to hold off cutting a 3.0.0 release until that is out.

@dpi thanks for the review!

Here's the changes you requested.

In theory 📌 Return early in EntityPermissionsForm::access if the user does not have "administer permissions" Fixed could fix this. Also being discussed there is the potential for removing the whole dynamic hiding of the permission local task all together (which is what triggers this issue).