🇬🇧United Kingdom @nicrodgers

Thanks for creating the MR @karengrey. We've been using the various patches in production successfully for years, however the issue is still tagged with NeedsTests so I'm going to set this back to Needs Work for now. Hopefully we can get some tests written and get this issue wrapped up.

We were experiencing this php warning in our behat test suite after upgrading from 11.x to 11.2

It's caused by a combination of old linux system (in this case, Debian 11) and PHP misbehaving.

On affected systems, you can reproduce the error by running

$ php -r 'var_dump(imageavif(imagecreatetruecolor(8, 8), "/tmp/test"));'
PHP Warning:  imageavif(): avif error - Could not encode image: No codec available  in Command line code on line 1
Warning: imageavif(): avif error - Could not encode image: No codec available  in Command line code on line 1
bool(true)

As you can see, php thinks it has avif support because it's returning true (you can also see this in the phpinfo output within the gd section) but the reality is that it's unable to create avif files because the codec is missing and gives that warning.

There's a bug on php.net for it:
https://bugs.php.net/bug.php?id=81217

The php bug has been closed as not a PHP bug - they say:

So, it seems that the root cause of this issue is that the libaom versions currently provided by Debian APT and Alpine APK systems are not functional (AFAIK they are compiled without the required BUILD_SHARED_LIBS=1, but I may be wrong here).
So, in order to make imageavif() and imagecreatefromavif() work, you need to do everything by hand:

1. compile and install libaom
2. compile and install libdav1d (to decode AV1)
3. compile and install libyuv (used by libavif to better handle YUV decoding)
4. compile and install libavif
5. compile and install the gd PHP extension (with the --with-avif flag)

For an example about how that can be done in Debian (so, Ubuntu too) and Alpine, you can take a look at https://github.com/mlocati/docker-php-extension-installer

Personally I think the php team are being dismissive here. Clearly it is a bug that could be better handled in php, because on the one hand php is saying YES, AVIF is supported here, and then on the other, when you try to use it it errors out because the codec is missing. But we are where we are.

For Drupal, I think the easy fix here is to suppress the warning from the imageavif() command within checkAvifSupport(). The function correctly returns FALSE on the affected systems so I think it's fine to suppress the php warning.

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

Updating to 11.2 has broken the `|render|striptags` workaround when checking if a views block is empty, as 11.2 views blocks now use placeholders by default. See related https://www.drupal.org/project/drupal/issues/3533588 🐛 Views blocks now use placeholders, making it impossible for the theme to determine if it’s empty or not Active and https://www.drupal.org/project/drupal/issues/3437499 📌 Create placeholders for more things Active

Caused by https://www.drupal.org/node/3523495 →

nicrodgers → created an issue.

Whoops - tried to update the MR to work with 11.2 but it seems to have gone wrong somewhere.. Sorry!

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

Tested and works - thanks!

@elc Huge thanks!

I've updated patch #9 to work with v2.2 (constructor function switched to setter injection).

Leaving as Needs Work because I haven't addressed the comments in #11. I wasn't sure how to make @tostinni's work to make ModuleHandler injection fall back to Drupal::service with this new setter injection method so I've left that for now. Help welcome :)

@daraven - hi, btw :-) huge thanks for taking on maintainership of this module.

@daraven maybe we should revert @nikolay shapovalov's commit in this issue that enables gitlab ci, and add it in a separate issue?

Whilst I agree having gitlab ci running automated tests would be a great position to be in, we don't have to do it as part of the Drupal 11 compatibility work. If we can get another pair of eyes testing the MR manually then we could get it RTBC'd and a Drupal 11 release shipped. We have been using the MR in production on various high traffic sites without issue for a while now.

Updated patch to support changes in D11 erroring when $node->id() is null.

updated the issue summary

The d10 compatibility patch from https://www.drupal.org/project/restrict_by_ip/issues/3289374 📌 Automated Drupal 10 compatibility fixes Needs review has been merged in to 8.x-4.x-dev so i've merged that branch back in to here and resolved the conflicts. This has reduced the number of changes in this issue quite considerably. All but one of the comments on the MR from the last review related to the D10 changes. I've implemented property promotion as per the last review in the relevant file. Setting back to need review.

Fixes the issue

nicrodgers → created an issue.

Maybe we'll need an update hook to set the value to FALSE for existing field instances?

nicrodgers → created an issue.

Setting back to Needs Work due to the problems highlighted by the pipeline with this new test. I've run out of time for this for today but may get a chance to come back next week.

Created MR based on patch number 4. Updated the info.yml to support Drupal 11.

nicrodgers → created an issue.

Rerolled to work with latest 2.x

Here's a static patch for those using a composer-patches workflow

Changing back to a bug as per #28

TODO steps as per #61

@deasly I like the idea - I think that would be better as a new issue, category Feature Request, so that it can be worked on separately to this.

I'd prefer to keep this issue as a bug because the behaviour unexpectedly changed between 2.0.1 and 2.0.2 because of the related issue.

Discovered this issue after updating to 2.0.2.
Unfortunately I don't think the change made here is correct. We shouldn't exclude unpublished terms, as the user may well have permission to view and use the unpublished term (the 'administer taxonomy' permission).

I've raised https://www.drupal.org/project/term_reference_tree/issues/3497889 🐛 Only hide unpublished terms from the node form if the user doesn't have 'administer taxonomy' permissions Active

nicrodgers → created an issue.

rebased to bring it up to date with the latest 11.x

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

rebased the MR to bring it up to date with 11.x

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

Here's a screenshot showing an example of the error page that is displayed instead of the nice Drupal themed 404 page.

nicrodgers → created an issue.

I've been testing this out, and the breadcrumbs are still duplicated on views pages. It looks like this is because the fix has been applied within the taxonomy term logic so for views pages it never gets hit and the default route params are still missing. Was there a reason this had to be nested inside the taxonomy term logic rather than being outside so it can apply everywhere?

I found that MR26 doesn't apply if you're using the Drupal 11 compatibility patch in https://www.drupal.org/node/3485520 → due to them both modifying the params in MenuBasedBreadcrumbBuilder::construct().

So i've re-rolled the attached patch, which enables you to patch this issue on top of the other one.

If this issue gets committed before the other one, then use the MR.

Setting back to needs work due to the failing coding standards issues picked up in the pipeline.

I've updated the issue summary, and created a merge request based on the patch from #71 as the patches have been deprecated.

Patch applies and upgrade status reports there are 0 issues after applying it.

nicrodgers → created an issue.

@maxilein are you sure you've applied the MR #5?
That is one of the things that's changed in it.

I don't see that code, or that error reported by Upgrade Status after applying it.

@pearls we are still using this MR on our sites and it's working fine:
https://git.drupalcode.org/project/drupal/-/merge_requests/715/diffs

@miiimooo we use content translation and workbench_moderation, so for us having that option would make no difference, we'd still need to use the core patch to improve the speed

nicrodgers → changed the visibility of the branch 3484874-filevalidateextensions-is-deprecated to active.

nicrodgers → changed the visibility of the branch 3484874-filevalidateextensions-is-deprecated to hidden.

nicrodgers → created an issue.

I've updated the MR to declare support for D11, as upgrade status was reporting that everything was compatible with no further changes required. I've hidden the patch files for clarity, the latest code is in the MR.

I've been debugging our case of this further, and it appears that our site is triggering it due to the same code mentioned in https://www.drupal.org/project/search_api/issues/3274158#comment-15802027 🐛 RuntimeException while trying to render item Active when it tries to build a form, it checks if the session has batch_form_state data set. If the session isn't yet started, it tries to start it, but NativeSessionStorage refuses to do so because the HTTP response headers have already been set (by the page's initial response).

This can be triggered in our case either by having the content moderation form show on the page (eg. moderate node from draft to published) or the autosaveform module.

However, unlike the report in #36, in our case, when this exception is triggered, the content is still successfully updated in the index, so it doesn't seem to be causing any real problems for us.

I'm still unsure as to why it would have only started happening since we upgraded from php 8.1 to 8.3 though.

FWIW - For us, these errors only started occurring after updating PHP from 8.1 to 8.3.

I think this can be closed in favour of https://www.drupal.org/project/file_delete/issues/3430549 📌 Automated Drupal 11 compatibility fixes for file_delete Needs review

I ran this patch through Upgrade Status for Drupal 11 compatibility, and all that was needed was adding ^11 to the info.yml, so I've done that. Now this issue can make the module compatible with D10 and D11. Seemed pointless creating a new issue for D11 and doing them separately. Setting back to Needs Review because of that.

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

nicrodgers → created an issue.

Patch 11 doesn't seem to be working with the current 2.x-dev version of the module.
The patch applies, but even after rebuilding caches, the additional attributes are not displayed. Only the standard three are displayed:

* target
* rel
* class

You need the D10 compatibility patch as well, from #3289374

Created an MR and attached a patch (same thing) with necessary changes for Upgrade Status to be happy.

nicrodgers → created an issue.

MR 5 applies and works fine with D11

MR15 no longer applies to 2.0.x-dev. I think it needs rebasing.

nicrodgers → created an issue.

nicrodgers → created an issue. See original summary → .

We can't use the DeprecationHelper without dropping support for Drupal < 10.1.3 because that's when the helper utility was introduced. Currently this module says it's compatible with 8, 9 and 10.

So instead of using DeprecationHelper we can do it the old-school way

if (version_compare(\Drupal::VERSION, '10.1', '>=')) {
  $logger = \Drupal::logger('pwned_passwords')
  Error::logException($logger, $exception);
}
else {
  // @phpstan-ignore-next-line
  watchdog_exception('lockr', $exception);
};

or we can drop support for Drupal <= 10.1.3

Duplicate of https://www.drupal.org/project/clamav/issues/3429235 📌 Automated Drupal 11 compatibility fixes for clamav Needs review

nicrodgers → created an issue.

Support from all Views contextual filter settings for the default_argument_skip_url setting is removed from drupal:11.0.0. No replacement is provided. See https://www.drupal.org/node/3382316 → .

We need to remove this from

config/optional/views.view.track_role_grants.yml
config/optional/views.view.track_role_history.yml

nicrodgers → created an issue.

I've fixed the 'not exists' typo and also added 'the' as a prefix to 'filesystem' to improve readability.

nicrodgers → created an issue.

Certainly is from us! Surprised other people haven't shown interest in it though.

Merged - thanks!

~2dareis2do - We are still using the patch from #30 in production on 10.3 . Haven't tried the newer patches.

I've re-read through every comment in this issue and have observed the following:

data-drupal-link-system-path

in #4, @nod said

li element shouldn't have the data-drupal-link-system-path either.

in #94, @andrewmacpherson said

What's going on with the <li data-drupal-link-system-path> attribute? In #4, _nod suggested removing it from the list item. It's still there though. It looks like an earlier patch (#26) removed it, but by patch #82 it's come back.

This hasn't been addressed, so we need to decide whether this is something that needs to be done here or not, and update the issue summary and description accordingly.

data-drupal-language

in #94 @andrewmacpherson also said

The new data-drupal-language attribute isn't very well explained.

and this hasn't been addressed yet either.

failing tests in #118 need addressing

needs re-rolling

I've tested MR 29 with Drupal 10.2 and can confirm it displays the helper text correctly, validates correctly and enables svg images to be uploaded.

I have not tested with Drupal < 10.2.

I have hidden MR 27 to make it clearer which MR is 'current'.

I reviewed the changes for coding standards, and have pushed a bunch of fixes to bring the files in line with Drupal coding standards.

I am not sure why so much code has been removed in the SvgImageWidget class, relating to settings and preview - is this intentional?

nicrodgers → changed the visibility of the branch 3413668-replace-upload-validators to hidden.

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

Thanks for the patch.

Is it possible to do that patch so that it works with both versions of scheduler?

If the patch fixes it with the latest version of scheduler, presumably it will break for people still using the previous version?
So to avoid releasing a new major version of advanced_scheduler I was wondering whether it was possible to make it work with both.

If not, we can roll this in to a new major version.

Doesn't apply to latest 2.x-dev - needs re-roll

Good spot Michalsen, I've removed those two now.

Removed the two unused Use statements.