🇩🇪Germany @ammaletu

This is still happening on Drupal 10.4. I had another look at this. Some details:

• The first patch above (#17) started with its own Layout-Builder-specific scrollTop command.
• Patch #20 removed most of that command, but not everything, and used the new default ScrollTopCommand instead.
• This doesn't seem to work, or at least not reliable. I got a JavaScript error "scrollTarget.get is not a function". Line 1882 in the scrollTop function does look to me like the jQuery call is missing.
• There is a ticket trying to radically simplify the scrollTop function: https://www.drupal.org/project/drupal/issues/3478087 ✨ Modernize Drupal.ajaxCommands.scrollTop() Active

I tried both versions, but none of them worked for the first error. If I hit "Submit" a second, theird etc. time, sometimes it works and scrolls up, sometimes it scrolls up a bit and sometimes it doesn't.

Does anybody know if this is specific to Layout Builder? Scrolling on the main part of the page is controlled by the Layout Builder to stay on the block currently being edited. Could this somehow interfere with our scrolling attempts here? If I have a good idea how to fix this, I'm more than happy to provide a correct patch in an MR.

If anybody is trying to reproduce it: For me this error happens when I am in the webforms admin page and click on "Add a webform". A modal is opened. If I now resize the window (e.g. by pressing F12 and opening the developer tools), the JavaScript error occurs. Drupal 10.4.5 and current Firefox.

That this fundamental bug is still open after 9 years is... oh well. Are we all just using "Easy Breacrumb" which handles this correctly?

Looking at the MR, there is the request to add type hints in some places. Makes sense, I added the requested type hints. Looking at the code, I also wonder:

• Shouldn't variables use camel case instead of snake case? Should it therefore be $cacheableTitle for local variables in TitleResolver?!
• Is Drupal\Core\Controller a good namespace for the new CacheableTitle class? I would have expected only controllers in this namespace. Then again, the TitleResolver is not a controller and is living there as well.

I could change the variable cases if somebody agrees this should be done. Then lets see what the pipeline says. I see the last run failed with stuff that has nothing to do with this MR (linting and spelling in JavaScipt files). Maybe somebody who knows how this works needs to merge the current 11.x state in here?!

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

We are running Easy Breadcrumb version 2.0.9 on Drupal 10.3.13. We have been using this module for years, without a problem. Then in version 2.0.7 something changed and some of our breadcrumbs did not work anymore. In version 2.0.8, this changed again and I again had to adapt our config. Now 2.0.9 is out, and our breadcrumbs are broken again. I consider our use case to be a really basic use case, and I don't understand how this can not be supported.

It's really simple: Some of our nodes have URL aliases and others don't. So what I want is:
* Node without alias: /node/123 => Home >> Node title
* Node with alias: /my-pretty-page => Home >> My pretty page
* Node with alias and longer path: /my-pretty-page/sub-page => Home >> My pretty page >> Sub Page

Which configuration worked so far:
* 2.0.6:
excluded_paths: node
custom_paths: ''
* 2.0.7:
excluded_paths: ''
custom_paths: 'regex!/node/([0-9]*) :: '
* 2.0.8:
excluded_paths: node
custom_paths: ''

The current changes came into effect with issue 3480899, I guess.

I tried the following:
* excluded_paths: node, custom_paths: '' RESULT: /node/123 => Home
* excluded_paths: '', custom_paths: '' RESULT: /node/123 => Home >> >> Node title
* excluded_paths: '', custom_paths: 'regex!/node/([0-9]*) :: ' RESULT: /my-pretty-page/sub-page => Home >> Sub Page

Whatever I do, one of the three scenarios doesn't work. Any help is appreciated!

I created an issue fork and committed the patch from #4 by tmiguelv. I fixed the comment to adhere to Drupal guidelines. For me, this works. but I also didn't analyze the logic flow in detail.

I see there are some functional tests already. Why aren't they picking up this behavior? With the patch applied, the second step of the form was setting up the trusted browsers. `TfaTotpSetupPluginTest` probably doesn't use this feature, so it is not a multi-step form. I think, `testPluginSetup` should be copied to a `testPluginSetupMultiStep`. Or should we add a new test for the trusted browser feature?

Same here as for tame4tex. Drupal 10.3.13 and tfa module 1.10.0. I had the TOTP method set as default, with a second method enabled. When I tried to set up the TOTP for a user, the code scan was requested twice. This stopped with the patch.

I think this is not relevant anymore in version 3.0.0 of the module and should be closed. There is no variable

$this->configFactory

anymore.

I activated the module on Drupal 10.3 with the changes from the MR, and the description of the field looks good, with the line breaks displayed as before. This can be merged.

Based on that - I would say the patch from #4 does the minimum to keep it consistent - but could introduce edge case regressions where a user with access to the admin theme may expect the batch to run in the front end.

I'm one of these users, and this change breaks something in our project. We have a form on a frontend page with a button to run a VBO action on the selected entities. The action renders them into a PDF, which should use the frontend theme as well. This worked previously as expected and still does for guest users. But if I'm logged in as admin, I get the PDF rendered in the admin theme.

So now the same action behaves differently, depending on who is running it. It's hard to understand why this change was even necessary. Is there any way to configure a specific bulk operation to use the frontend theme?

I corrected the doc block in the api file. There is currently only one test in this module, so I'll be bold here and set this to "needs review". :-)

I created a merge request with the proposed fix. I'm not a 100% sure that this is indeed the best solution. There are still three or four places which use 'confirm' in the module. Changing the key here to 'subscribe_combined' works, I just tested it in the 4.1.0 version. So somebody with a better understanding of how the module works shoudl have a look.

Also, this might need a test if it could break without anybody noticing. :-) Looking at the existing tests, it seems this would belong into "SimplenewsSubscribeTest". Maybe as a variant of "testSubscribeAnonymous"?!

Ok, wow, I did not expect someone to work on this within minutes. :-) Should I simple close my MR again?!

ammaletu → created an issue.

This is still happening in Drupal 10.2, and it is hard to justify to customers why they have to save a content or clear the complete cache when they create or change a URL alias through the alias admin page.

It seems to me that having a cache tag like "node:42:path_alias" would be nice. This should be used whenever the URL of a content is used anywhere, even if there is no path alias yet. Whenever a path alias for this content is touched (newly created, altered, deleted) this cache tag could be invalidated.

As a remedy for now, I implemented three hooks hook_entity_insert, hook_entity_update, and hook_entity_delete in a module. I check $entity instanceof PathAliasInterface and if yes, then call a method which checks the path and tries to guess the correct cache tag to invalidate. Not perfect, but works for the common cases of node and group content URLs.

/**
 * Implements hook_entity_insert().
 */
function my_module_entity_insert(\Drupal\Core\Entity\EntityInterface $entity): void {
  if ($entity instanceof \Drupal\path_alias\PathAliasInterface) {
    invalidateCacheForPath($entity->getPath());
  }
}

function invalidateCacheForPath(string $path): void {
    // Try to guess the correct cache tag.
    $cacheTag = '';
    $matches = [];
    if (preg_match('@^/([a-z_-]+)/(\d+)$@', $path, $matches) && count($matches) === 3) {
      // Handle the default case of a URL like '/entity_type/id'.
      $cacheTag = "{$matches[1]}:{$matches[2]}";
    }
    elseif (preg_match('@^/group/\d+/content/(\d+)$@', $path, $matches) && count($matches) === 2) {
      // Handle the special case of group content.
      $cacheTag = "group_content:{$matches[1]}";
    }

    // Invalidate this cache tag.
    if ($cacheTag) {
      \Drupal::service('cache_tags.invalidator')->invalidateTags([$cacheTag]);
    }
}

I could not reproduce this with Drupal 10.2.10 and Simplenews 4.1.0. Is there a reason why the previous version 4.0.0 ist still listed as "Recommended by the project’s maintainer"?

This happens in our projects, too. It is especially bad for subscribers without an account, as they can't simply go to their user profile and unsubscribe from the newsletter. I haven't tested yet if this happens for subscribers with an account as well.

In our case, the unsubscribe link is the only link in the mail to manage the subscription. That effectively leaves no way to unsubscribe for the users, except writing to the admin. This probably has legal consequences, if we keep sending newsletters to users who don't want to receive them.

This just happened on Drupal 10.2.7 and Simplenews 3.0.0-beta5. I still have to test whether this also occurs with the new version 4.1.0.

@spuky (see #6): I think I got this to work now. I did not use "Paths to be excluded while generating segments". As soon as that contains "node", no breadcrumbs are generated anymore, because there is nothing in front of "node" to generate a segment.

Instead, I used "Paths to replace with custom breadcrumbs" with:
regex!/node/([0-9]*) :: <title>

That seems to do the trick.

Another question is, why this is even necessary for nodes? This works out of the box for groups (/de/group/123) and for users (/user/42). They each get a breadcrumb like "Startpage > Unlinked Title". Only nodes get "Startpage > > Unlinked Title". So maybe the real bug is this empty segment for nodes?

I just ran into this on Drupal 10.2.8. It seems to happen always once you logged in as a user who can see the toolbar and then log out again. After comparing this with a clean Drupal 10.2 installation with Claro theme, I found the source of the problem: A colleague required the "toolbar/toolbar" library as a dependency in our theme, unconditionally. This way, the toolbar.js file was always loaded, even when no toolbar was being displayed.

I added steps to reproduce in the issue summary. This is more or less a user error, but maybe it helps somebody else in the future. Of course, it would be nice if there was a way to clear this session storage value when switching sessions. Maybe add something after the "Add anti-flicker functionality." block, starting in toolbar.js on line 241. If "#toolbar-administration" is not a part of the page, can we then delete the session storage value? This would help in cases where this library is loaded withput a toolbar.

I tested the change and it works perfectly! Users with more text formats still see the dropdown without the webform text format. And my registered users without any other text formats don't see the Plain text select anymore.

Thanks a lot for the quick patch! I just added two comments to the MR, concerning typos in comments.

I didn't want to outright label it as a bug, although I do think that it is one. There already is an unanswered question on Drupal Answers: https://drupal.stackexchange.com/questions/320170/what-is-the-purpose-of...

From a practical point of view, the whole thing boils down to "Do webform users need this text format or only webform admins"?

ammaletu → created an issue.

I don't think this is about treating the exclusion string as a regex or not. The same thing happens if I put a regex in the exclude config instead of simply the string "node". I just tried it with "node\/[0-9]+" and still didn't get breadcrumbs beyong the Home link.

The question is against what part of the URL the regex is run. If you actually want to run it against the whole URL, then there's no point in doing it more than once. But since the exclude feature is there to remove parts of the URL, this doesn't sound right to me.

Also, I noticed that there doesn't seem to be a test case for the exclude feature.

ammaletu → created an issue.

The patch works for me, but you need to add it in both "showNextButton" and "showBackButton".

These two patches look simple enough and should be committed, I think. I also wanted to ask if there will be another release of the 2.x branch? The last alpha5 release is from more than a year ago. I'm pretty sure that at least some bugs have been fixed since then, and I would hate to run into issues which have been solved months ago, when we finally upgrade the group module to version 2.

For me, this patch did apply to the beta4 version and fixed the problem. Using Drupal 10.2.5 and PHP 8.1.27.

Please, commit this patch and create a new release! I spent an hour chasing this bug, when the patch solving the issue had been published two years before I even started working at my current company! If there is anything else that we can do to help speed this along, please tell us.

I can confirm that the Merge Request as it is fixes this issue in Drupal 10.2.5. We just upgraded from CKEditor 4 to 5 and had extra linebreaks whenever the source contained a linebreak right inside the paragraph (not sure how our customers managed to get the linebreaks there).

This issue turns 10 years old in just a couple of months. As far as I can see, the MR title needs to be changed and one change to the test case needs to be reverted. I tried to do that, but Gitlab wouldn't let me.

I had another look at the test I wrote and fixed some errors. The test looked OK, but wasn't actually testing anything because of an error in the routing configuration. To me, this looks OK now and I verified with a real module and hook that the original problem still shows up in Drupal and was not fixed in some way.

But still, the test succeeds even without the fix in `core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php`. I'm a bit lost why that is. Is something missing from the test form setup so that the bug is not triggered? Maybe somebody else has an idea? For now, this remains on "Needs work".

Once this is fixed, it would be nice to tag a new release. We just migrated from CKEditor 4 to version 5, and this makes this button harder to use for German users.

Ammaletu → created an issue.

This seems to work in Claro, because `Drupal.behaviors.entityBrowserAddThrobber` is called, but it doesn't go into `if (context === document) {`. In our own theme it does, though, and thus adds a second throbber, after a first one has been added and removed through the proper methiods (briefly, virtually impossible to see withoput debugging).

I see that `Drupal.behaviors.entityBrowserAddThrobber` has just been added in version 2.10.0 (see commit https://git.drupalcode.org/project/entity_browser/-/commit/0a3fdfcf801b8...). I'm just not sure what the IF check is supposed to do and why this works in some themes and not in others.

We have the same problem: Since the update to version 2.10.0, the throbber is added but not removed. This makes sense, since there is no code removing it. In other instances, the throbber is set through `Drupal.Ajax.prototype.setProgressIndicatorThrobber`, where a reference to the element is saved and would later be removed. The entity browser module simply adds the HTML code for the throbber through `Drupal.theme.ajaxProgressThrobber`. This doesn't generate a reference in `this.progress.element`. The entity browser module itself has no code to remove the throbber again, as far as I can see.

Should we open a new ticket for this?

I ran into this today and can confirm that the patch from #5 is fixing this. I also think that haven accessCheck(FALSE) is the correct behavior. The way it is now, permissions can not be saved for a group under Drupal 10.1+, which is after all the whole point of this module. :-)

It would be great if you could tag a new release for this. It would be even better if this could be ported to the 1.x branch as well. The concerned class looks the same, so the fix can be applied to both branches without modifications.

I fixed the linting issue.

Ok, I tried to put the previous issue summary inside the issue summary template and added some lines. I have removed the two tags and set this to "needs review" so that somebody can have another look at it.

I just ran into this bug when I was trying to resort form elements and it seemed really simple. I created a branch, pushed the patch from #13 and added a simple test case. The test failed without the patch and succeeded with it.

This is my first time contributing code with a branch, so help me out a bit with the next steps. :-) #15 mentioned an issue summary update. Not sure what is missing there. Also, does this need a change record? And then set it back to "needs review"?!

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

I'm interested in getting this finished and merged. Currently, I have a class-based block which uses the pager manager to get unique pagination IDs. This works, even if the same block is used more than once on the same page. But as soon as a views-based block is used on the same page (via Layout Builder), this breaks and they both share the same pagination.

What is needed for this ticket to move along? An update hook initializing the new flag for all existing views?

We just stumbled upon a new variant of this bug (Drupal 10.1.7): Our users have the "administer blocks" permission, but not the new per content block permissions nor the "Access the Content blocks overview page" permission. Everything worked fine for them with Drupal 9. Since the update, they are not able to add media to a block anymore.

The error message which is logged when clicking on the "Insert" button in the media modal (no error shown to the user):

Path: [URL with many parameters]. Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException: The following permissions are required: 'create gallery_widget block content' AND 'access block library'. in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 118 of /var/www/html/web/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).

The change record → for the new permissions sounded as if they were optional, as long as users have the "administer blocks" permission. Now it seems, this has changed so that you do need the new permissions for adding media items to a block!?

We see a very similar behvior in our Drupal 10.1.7 sites. Every time I let go of the handle for resizing, the Layout Builder panel jumps a bit to the left. Apparently, its position is computed and then set as "x pixels from the left". If the panel is supposed to be shown on the right side, why is it not positioned as "y pixels from the right" (y being the width of the panel)?

@Ammaletu Is the bug reproducible without contrib or custom code? If so, can you provide steps to reproduce?

I'm afraid not. I had another look, and we got the "duplicate UUID" problem because we are caching the form state (to solve yet another bug). Invoking the cache kill switch on the pages with the form solved this for guest users, except for one form which is displayed via AJAX. For this last form with the "duplicate UUID" problem, the patch from this issue helped. I'am afraid I don't have the time and expertise to boil this down into a generic use case.

I just tested the patch from #2 and all kinds of bugs went away. It's hard to imagine how much headscratching and confusion could have been avoided with this being included in core three years ago!

My setup is Drupal 10.1.7 with PHP 8.1.

The patch solved this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field. Display the Remove button.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Remove one of the nodes.
7. Remove another one. Instead of removing the node, the modal window to add more nodes opens.

It also solves this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Try to save the block, but provoke a validation error, e.g. by saving the block without a title. The Layout Builder is reloaded, but the previously selected nodes are not displayed anymore. They are back again if you add another node or if you simply save the block without seeing them.

It also apparently solves a bug we had where a cached form_state was reused when different anonymous users opened the form at the same time. Only the fastest of these users then could save the form, the other's got a "duplicate UUID" error.

So, how do we add a test for this in a generic way, independent of the entity browser module? And are we sure that the approach in the patch is the right way? Is there a way to get a core committer to have a look at this? To me, this seems like quite a fundamental issue, causing numerous seemingly unrelated and hard to debug bugs. Not some minor issue that can easily be patched by community members.

I just updated to webforms module version 6.2.1, and now our deploy process stops at the first step, with the database updates:

> [notice] Update started: webform_post_update_authenticated_user_permission
> [error] Adding non-existent permissions to a role is not allowed. The incorrect permissions are "use text format webform_default".
> [error] Update failed: webform_post_update_authenticated_user_permission
[error] Update aborted by: webform_post_update_authenticated_user_permission
[error] Finished performing updates.

Any ideas how to get this working?

I know what happens, and good news: This can easily be remedied on your page. I also can't stop laughing at how easiy this is once you understand it and how dumb Drupal behaves here.

What happens is this:

1. You have a content type, e.g. the basic page, and set up its fields. At some point you add the "comments" field to it and Drupal will add it to the view display of the content type. If you view an entity of this type, the comments will be rendered and everything is fine.
2. You decide that you want to use the Layout Builder for this content type and activate it on the content type's "Manage display" page. All the fields that you previously configured here are now gone and you have an empty Layout. You fill the default layout of the content type and also add the comments block to it. The page for an entity of this type is rendered with the blocks yoo configured in its Layout, and everything looks fine.
3. The fields that you previously configured are not gone, though, they are just not visible anymore. You can see this when exporting this content type's configuration. Look into the file, e.g. "core.entity_view_display.node.page.default.yml", and you'll see the old fields under the "content" section and the new Layout Builder blocks in the "third_party_settings" section. For a brand-new content type you can also test this: Enable the Layout Builoder, disable it again and see that all your previous fields are still there. For established content, disabling the Layout Builder is not allowed anymore.
4. Now, when Drupal renders the page, it apparently first looks at the configured fields in the "content" section and renders them. Then, the Layout Builder comes into it. It throws the previously rendered content away, looks at the configured blocks and renders them. The latter is was you see on the page.

This is quite a wasteful approach, and I'm sure that there should be a way to recognize early on that this content uses the Layout Builder and therefore nothing in the "content" section of its view display needs to be rendered. This ticket should probably be renamed accordingly.

For the workaround: Export the configuration, move the comments from "content" to "hidden" and re-import it.

This seems to go like this:

1. CommentStorage::loadThread is called twice, from some kind of pre-render hook. I'm not yet sure why.
2. The loadThread method extends the query with a PagerSelectExtender class, which handles the actual pagination. Only the class name is set. When the PagerSelectExtender is instantiated, its element member is not set.
3. When the query is executed, PagerSelectExtender::execute is called, which in turn calls PagerSelectExtender::ensureElement. Since no element is set, this sets the element member to "$this->connection->getPagerManager()->getMaxPagerElementId() + 1".
4. On the first run of loadThread, this results in element=0, since the default value of the getMaxPagerElementId getter is -1.
5. On the second run, the first run already registered the pager with the pager manager and the element ID 0, so by adding 1 we are now getting element=1.

The question remains, why only when using the Layout Builder the pre-rendering of the page calls LayoutBuilderEntityViewDisplay::buildMultiple (line 268) twice. I think, the next step should be a test case showing that problem.

In the meantime, I had a second look and understood that I need to install a Chromium driver so that the JavaScript tests are not simply skipped. With ddev-selenium-standalone-chrome installed, they are now executed. In my local test setup of a fresh Drupal 10.2-dev installation, JSWebAssertTest works with the patch installed as does MediaTest.

Can somebody with a better understanding of how Drupal's test setup works please have a look at what might be missing here or if we can set this back to RTBC? I'm setting this back to "Needs review" for now.

Also, comment #44 raised the question "if locale_translation_batch_version_check needs a change record". I'm not sure about that -- do we need a change record just because there is a new function? But I think fixing the bug might warrant a change record, as people might rely on the old translations or have lots of missing translations added themselves.

It also happens on 10.2.0-dev. pagination links go to "page=1,0" for the second page. If a pager ID other than "0" is set on the comments block (e.g. "1"), the links go to "page_id=,1" and then everything works.

This is still happening on Drupal 10.1.6 and 10.1.7-dev.

I guess for this to be merged we need a test, right?

I would be interested in getting this finished and merged. This might not be the worst bug that Drupal has to offer, but it still breaks a pretty basic core functionality. Right now, people are upgrading to Drupal 10.1 and will be wondering why all kinds of simple and visible Strings are not translated.

I just added the patch #43 to our Drupal 10.1.6 installation. It applied without a problem and cured the problem with the missing translations of newer Strings. The new test LocaleTranslationChangeProjectVersionTest.php runs successfully, as does the changed test LocaleUpdateCronTest.

I'm not sure about Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest that apparently failed back in May, see #45. Our project still uses CKEditor 4, but I tried it on an unmodified Drupal 10.1 installation. Everything worked there, but all test methods in the MediaTest were skipped.

So what is actually mssing here? I tried to trigger a restest of patch #43. When this hopefully succeeds, can we simply set this back RTBC?

I would very much appreciate this addition. The ExtraLinks class unfortunately adds some links which are unncessary and duplicate, and I haven't found a built-in way to remove them.

It works with the patch #2 and a hook like this:

/**
* Implements hook_admin_toolbar_extra_links_alter().
*/
function mymodule_admin_toolbar_extra_links_alter(&$links): void {
unset($links['media_library']);
}

The two lines in #3 indeed fix this issue -- but they also potentially open up a new, horrible bug! We added these lines, and now when more than one guest user opens a page with this form, only one of them can submit the form (whoever is quickest). The rest get a "Integrity constraint violation, duplicate entry for node_field__uuid__value" error. This only happens for guest users, because for them the whole page is cached. As I said, a horrible bug for end-users and hard to spot when testing alone.

Think very hard before simply adding these two lines. Are these forms which are displayed to guest users and do you have the static page cache enabled? Not caching the form and using a unique form ID might be the better solution. We will for now try to simply not cache pages which have these forms. Together with the cache kill switch, the fix from #3 seems to work.

I can also confirm that patches #30 and #31 solve this issue. Please merge this and tag a new release. This is a beaking bug in a module that claims to be compatible with Drupal 10, but does not actually work with Drupal 10.1. And depending on where on your page the rate module is used, this might actually be hard to spot.

@Anaconda777 Patch 3260094-#32 applies for me, at least the version of it that we have committed in our repository since April 2023. Some bits of it look different, but in the end it's a 5-character-change. :-) Patch 3372528-#6 did not apply, but it was easy to look through the files and see which lines were not changed. Somebody rerolled this an hour ago. Please test the patch from 3372528-#13 and leave a comment there if it works for you.

It's fixed in the upstream version, see line 376 of: https://github.com/h5p/h5p-editor-php-library/blob/master/h5peditor.clas...

@catch I just tested the dev version under Drupal 10.1-dev and PHP 8.1. I didn't find any errors, once I installed these two patches:
* #32 from https://www.drupal.org/project/h5p/issues/3260094 🐛 PHP 8.0 deprecated syntax warnings for h5p-editor RTBC
* #6 from https://www.drupal.org/project/h5p/issues/3372528 🐛 Drupal 10 compatibility JS aggregation causes js error with aggregation enabled RTBC

Against the current dev version patch #6 almost works. The changes in src/H5PDrupal/H5PDrupal.php in line 88, 89 and 94 don't apply, I think. After I changed these lines manually, displaying the H5P content worked again. Patch #8 contains a lot of the changes to get H5P to work with Drupal 10, which are in some way already part of the dev version, soon to be tagged as alpha4.

I just tried the current dev version of H5P with Drupal 10.1 and PHP 8.1 and got these deprecation notices. The patch from #32 fixes them.

I also got the development branch and checked it under Drupal 9.5.11. No problems so far, although we don't use that many H5P modules. I will test this with Drupal 10 later when I have the time.

This works for me. We had a patch like this in production for month now, using PHP 8.1.

I think this is a duplicate of https://www.drupal.org/project/h5p/issues/3281812 🐛 json_decode(): Passing null to parameter #1 ($json) of type string is deprecated Needs review .

There seem to be two Merge Requests trying to make H5P ready for Drupal 10. This one, and the one in https://www.drupal.org/project/h5p/issues/3348114 💬 Drupal 10 support Needs review . Would it make sense to combine the two? Or better to simply get one of them ready and then see about cleaning up the other one?

Is there anything I can do to move this along? We want to upgrade to Drupal 10 in a few days, and now I see that we can't without heavily patching this module. End-of-life for Drupal 9 is in 8 days! Please let me know what the next step would be. I'm more than happy to help out with testing this and getting the MR ready.

There seem to be two Merge Requests trying to make H5P ready for Drupal 10. This one, and the one in https://www.drupal.org/project/h5p/issues/3329297 📌 Automated Drupal 10 compatibility fixes Needs review . Would it make sense to combine the two? Or better to simply get one of them ready and then see about cleaning up the other one? The other MR has at least one patch which is missing here (see comment #14 in the other ticket).

Is there anything I can do to move this along? We want to upgrade to Drupal 10 in a few days, and now I see that we can't without heavily patching this module. End-of-life for Drupal 9 is in 8 days! Please let me know what the next step would be. Incorporating Pjotr's comments into the MR?

Thanks, quietone, for the review! And thanks vasica for doing the recommended changes!

"The CR uses the term 'technical name' for the machine name. I have not seen that before so I think it should be changed." -> Done.

Joachim Namyslo → credited Ammaletu → .

As requested, I wrote a change record for this. Also my first time, so somebody please have a look if this is appropriate in tone and length: https://www.drupal.org/node/3353397 →

Thanks for your comment, smustgrave! I added the new exception to the two installer interfaces, created an interdiff to patch #88 (which I based this on, as I understand that #89 went into the wrong direction?!) and added a second patch with only the changes for the new test cases.

This time with correct indentation. Sorry for the confusion. It's my first time working with patch files.

Ok, let's try this again. This time including the newly added files. :-)

I finally found the time to update the patch from #88 to work with the current 10.1.x branch. It mainly involved adding the test methods to the correct test classes. The two new test methods fail without the modifications and pass with them.

Our company just stumbled upon this issue. Took us an hour or two of valuable time to understand this, and we almost broke a customer website over this. :-(

I want to help get this patch merged. I haven't contributed to Drupal so far, so I need some advice. In which branch should this be fixed? "10.1.x" because this is the currently developed version? Or "9.4.x" because this is the last suppported version? Then I'll try to get the patch from #88 working with the comments from #94.

No problem. The company I work at is using Drupal for our product, so giving back to the community a bit feels good. Thanks for creating the new release!

I checked this again and debugged it a bit. The date formatter correctly outputs `Di., 14.02.2023 - 18:49` for the current timestamp. When I use this as input for the t function's placeholder `:date`, only the 49 is in the output. Everything works as expected when using `@date` as placeholder.

The documentation for the t function placeholders list the different approaches:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend...

It states for : placeholders: "Return value is escaped with \Drupal\Component\Utility\Html::escape() and filtered for dangerous protocols using UrlHelper::stripDangerousProtocols()." The latter function interpretes everything before the minute as a protocol, and since it is unknown it is stripped out. I think in this case using @ for the placeholders is the better approach.

I tested your changes. Reverting the permission revision works now. There is something strange about the message which is set, though. I got `Copy of the revision Test 1 from 46.`. The last part is supposed to be the date, but only shows the minute of the original revision which was reverted (saved at 18:46). I tried with different revisions and always got the minute from the original timestamp.

Ammaletu → created an issue.

Thanks for the quick fix. I tested your changes locally -- they work well and avoid the previous error.

Ammaletu → created an issue.