🇺🇸United States @bkosborne

I created a new MR that reverts the previous hotfix and implements what I think is simpler solution. The problem is that we're using ->appendXML to attach HTML to the DOM Document. This isn't really correct, because what's valid in HTML may not be valid in XML. That's why we're running into issues with &, which is perfectly valid in HTML but it must be escaped to & in XML.

I think the real solution here is to find a way to properly attach HTML to the DOM Document instead of using ->appendXML. Short term, we can cover most use cases by simply escaping the invalid XML characters. I say this is short term because we're using ->appendXML to append the rendered tooltip, and I don't think we can really mess with the rendered output by escaping it.

Note that for the test, I fixed the mock render code to actually perform a simple version of twig autoescaping.

bkosborne → changed the visibility of the branch 3351345-omdocumentfragmentappendxml-entity-line to hidden.

So, on 3.0.3, with the hotfix from this issue, I have problems with parsing this scenario:

• Glossify term "testing"
• Glossify term "PB&J"
• Text that reads "I am testing a sentence with PB&J in it"

I get output "I am testing". The rest is cut off entirely.

When I downgrade to 3.0.2, everything works as expected, and I don't get the original PHP warning that this issue posted about. I'm on PHP 8.1.x if that matters at all?

Can we have clearer steps to reproduce the problem? As it stands now, the hotfix in 3.0.3 didn't fix anything for my situation, but broke it. Hmm.

I just experienced this too. It can cause text to be completely left out in a scenario where you have two glossify terms in one text segment, and the second term has an ampersand in it. All the text after the first term is not output at all :(

I'll look at the code, though it does seem pretty complicated.

I tried verifying the steps in issue summary and I cannot reproduce the problem. I don't receive any error after restoring an article from the trash. I tested this on a clean 10.3.x site install.

Can anyone else verify the issue, and maybe update the issue summary with what the problem actually is?

This is fine, we'll just need to be real clear about it in the release notes. Thank you

It didn't occur to me to test in Chrome, thanks for that! I guess due the nature of the issue I didn't think it would be browser specific. Thanks for doing that and the MR! I'll test this next week and report back...

I added 🐛 Form cache causes issues with media library widget Active which I think might be related to this.

I added 🐛 Form cache causes issues with media library widget Active which I thought might be related, but the patch here does not resolve it.

bkosborne → created an issue.

Okay, this will need tests obviously, but I think this approach could work. I'm a little uncertain on if I should be passing the complete form in or not as the second argument to FormAlterHelper::alterForm. But in my brief testing this works.

Confirmed this is an issue as well.

The form_alters that Workbench Access performs assume that the form a property entity form:

function workbench_access_form_alter(&$form, FormStateInterface $form_state) {
  $form_object = $form_state->getFormObject();
  if (!$form_object instanceof EntityForm) {
    return;
  }
  $entity = $form_object->getEntity();
  if (!$entity instanceof ContentEntityInterface) {
    return;
  }
  \Drupal::classResolver()->getInstanceFromDefinition(FormAlterHelper::class)->alterForm($form, $form, $form_state, $entity);
}

But the form that the Media Library module uses is actually a custom form called Drupal\media_library\Form\FileUploadForm. That form embeds an entity form within it, but doing so does not invoke form alter hooks for the entity form.

I think Workbench Access needs to add some special handling for these media library forms. I'll add a merge request.

Hi, thanks for the offer! If you provide some support in moving current issues forward and getting them ready for merge, then I can consider it. Thanks again

bkosborne → created an issue.

Hid the patch, and incorporated it into a merge request. I also hid one of the existing branches from the fork.

This looks good, but needs tests.

bkosborne → changed the visibility of the branch 8.x-2.x to hidden.

bkosborne → changed the visibility of the branch 3316757-hide-language-column to hidden.

This is a duplicate of ✨ Remove the language column if all the columns have the site's default language RTBC and the latest patches on each are identical. Closing this in favor of that one, which was opened earlier.

bkosborne → changed the visibility of the branch 3316757-hide-language-column to active.

Setup tracking of usage of both block_content and media, and the media usage data will link straight to the node the inline block is on.

This works, but for sites using layout builder heavily, this adds a ton of additional tracking data. I have a site that only has "Media" as a source entity type to track, and I have ~40,000 items in the entity_usage table. Adding the Custom block as a source entity type balloons this to ~63,000 items. (this is mostly due to the large number of revisions we have on the site, each of which contain lots of inline blocks).

I wonder if there's a better option out there for this. We previously hacked something together where we issued a query to the inline_block_usage table to determine the proper source entity. This resulted in an on-demand query for each "row" in the result table though.

bkosborne → created an issue.

bkosborne → changed the visibility of the branch 3396987-issue-event-for to active.

bkosborne → changed the visibility of the branch 3396987-issue-event-for to hidden.

looks good

Thanks for verifying. I'll merge and put out a new release.

I agree this could use improvement. I think that the first option presented is the only one that really improves it. The issue to solve here is that as AP Style Date currently presents things, someone might think that the timespan applies to each individual date in the range. E.g.,

Currently displayed as:
Oct. 18 to 20, 2024, 10 to 11 p.m.

Interpreted as:
Oct. 18, 2024, 10 to 11 p.m.
Oct. 19, 2024, 10 to 11 p.m.
Oct. 20, 2024, 10 to 11 p.m.

But whoever entered the date may not have meant it that way, and instead meant it as:

Oct. 18, 2024 10 p.m. to Oct. 20, 2024 11 p.m.

What the actual correct thing to do is hard to say, because two different content editors might have a different understanding when they enter the dates.

But I think the safer way to display this kind of range is what's recommended as #1

Okay, thanks for explaining. Sounds good. I agree that requests seeing these errors will be bots.

Sorry this is so frustrating. There doesn't really exist a simple solution for adding 3rd party library dependencies to Drupal modules. This module was basically forked from another deprecated module, Lightning Media, so I mostly just copied how things were done there.

In any case, this module has a dependency on the DropzoneJS module → . That module's page has information on how to to add the Dropzone JS library. It seems you did follow those instructions but are still having an issue. Can you try downloading version 5.7.2 of that library and seeing if it works? That's the version I use successfully.

After updating symfony/http-foundation to 6.4.14, I started experiencing out of memory errors for requests for paths like this:

/+/testing

With symfony/http-foundation 6.4.13, a request like that on my site would show the normal 404 page. But 6.4.14 has this in Request.php:

        if ('' !== $uri && (\ord($uri[0]) <= 32 || \ord($uri[-1]) <= 32)) {
            throw new BadRequestException('Invalid URI: A URI must not start nor end with ASCII control characters or spaces.');
        }

I guess the + is interpreted as a space so that exception is thrown. Then I get an infinite loop as described here and eventually an out of memory error.

However, with this patch, applied, while I no longer get an infinite redirect loop, I also don't get the normal 404 page anymore. Instead, I get a page that just has message No route found for "GET https://mysite.com/+/test"

Is that expected behavior with this patch? that the 404 page wouldn't be shown anymore for bad requests? I guess that makes sense.

So in this cases you actually would like to expose a Call To Action based on a deliberate decision and driving the user.

But I can't do that anymore with this change, right? At least, not using this field formatter.

Isn't there a legit use case for still linking to pages a user does not have access to? Like if the entity being linked to is not available to anonymous users, but is available to authenticated users. We'd still want the link to that entity because we can log the user in automatically when they encounter a 403 and are logged out.

Hi - can a new release please be tagged for this?

bkosborne → created an issue.

I don't think so. Then the connect timeout isn't really being respected if we try again, right? 10 second timeout can become a 20 second timeout.

This is a nice simple adjustment.

Absolutely, I could use the help. I've added you as a maintainer. Please ping me on Slack before cutting a new release that's D11 compatible.

bkosborne → created an issue.

bkosborne → created an issue.

Fixed the broken test

bkosborne → created an issue.

bkosborne → changed the visibility of the branch 3480527-ssl-verification-support to active.

bkosborne → changed the visibility of the branch 3480527-ssl-verification-support to hidden.

Ohhh yes, a template element seems most appropriate here.

I'm pretty sure that won't fix the issue, it's due to that core issue, it will happen immediately or on cron, I had to remove the rendered item indexing entirely.

This core issue? 🐛 NativeSessionStorage Failed start Postponed: needs info

It doesn't look like there's a bug in core there. Core is throwing an exception due to a problem created by some scenario/configuration of a site.

Updated the MR to be based on 3.x

Looks like tests were failing due to an infra issue. But we did have some deprecation and code style fixes. So I took care of that.

Pushed an update that uses HttpExceptionSubscriberBase and preserves the destination parameter. Updated issue summary to describe the change.

I'm a little confused why this was committed and pushed out as a release when it's not officially adopted as a coding standard yet? I think I just made an assumption that coder only includes things that are part of the coding standard. But I guess they can't always happen simultaneously?

bkosborne → created an issue.

I tested my steps above and they didn't work. When re-indexing, I was getting errors like "cannot change field "xyz" from index options=DOCS_AND_FREQS_AND_POSITIONS to inconsistent index options=DOCS_AND_FREQS_AND_POSITIONS_AND_OFFSETS", just like the release notes for Search API Solr 4.3.x → indicate I would.

I had to delete the index from Acquia Cloud UI, re-create it using the new configset, then reindex the site.

I know there was some discussion if this needed a change record or not, and ultimately one was added.

I just want to ask that any changes that affect APCu cache always do receive a change record. I help maintain a very large (> 500 sites) multisite install. Any increase to APCu cache utilization, however small, can have cause a big increase in my overall APCu utilization and cause major issues if all allocated memory is exhausted. I don't say this to indicate Drupal shouldn't utilize APCu cache for new things, just that the core committers please ensure that the changes are advertised as change records, just like this one was. It allows me time to test how much more memory I'll need to allocate. Thank you

This started for me after upgrading from Drupal 10.2.x to 10.3.x due to this change: 📌 Remove calls to Request::hasSession() Active .

For me, it happens when indexing a rendered_item field on a node that has a form on it (and I have immediate indexing enabled). FormBuilder::buildForm has this chunk of code:

    if ( $request->getSession()->has('batch_form_state')) {
      // We've been redirected here after a batch processing. The form has
      // already been processed, but needs to be rebuilt. See _batch_finished().
      $session = $request->getSession();
      $form_state = $session->get('batch_form_state');
      $session->remove('batch_form_state');
      return $this->rebuildForm($form_id, $form_state);
    }

So 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).

That code should probably check that the session is actually started before checking if it has data, but I'm not really sure.

In any case, I think I'm just going to disable immediate indexing. This could quickly become wack-a-mole.

A problem here is that while the exception is now being caught by Search API and logged, the item isn't re-added to the queue (understanble, since Search API can't know if the exception will continue to happen for ever) to let cron processor take it on. So the item never gets re-indexed.

I've run into a similar problem, but my node doesn't contain a view, but instead contains a form. FormBuilder::buildForm has this chunk of code

    if ($request->getSession()->has('batch_form_state')) {
      // We've been redirected here after a batch processing. The form has
      // already been processed, but needs to be rebuilt. See _batch_finished().
      $session = $request->getSession();
      $form_state = $session->get('batch_form_state');
      $session->remove('batch_form_state');
      return $this->rebuildForm($form_id, $form_state);
    }

So 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).

That code should probably check that the session is actually started before checking if it has data, but I'm not really sure.

Created 🐛 Warnings logged for missing field block plugins when enabling layout builder on an entity view display for the first time Active

bkosborne → created an issue.

With this feature module disabled, I'm seeing log messages like this when enabling layout builder on an entity view display of any entity type (tried both node and taxonomy)

The "field_block:taxonomy_term:test:description" was not found

It's just a warning and things are fine (probably after block plugin caches get invalidated somewhere).

It's from BlockManager being unable to find the block plugin definition. I suppose there's some cache race condition here contributing to this. When you enable layout builder for an entity view display, it creates the default layout view display and places a field block plugin for each field. But since we're not exposing field block plugins until the entity view display is converted to layout builder.... yeah.

I had the same situation as #19. Got this error in my CI when running a large behat test suite, where a test deleted a user role that was created. Thanks for fixing this.

Included in 3.0.0-beta2

Included in 3.0.0-beta2

Been using this in production for a few months

Something going on with the MR. There's way too many changes in there. Can you take another look?

Sorry all, I'll get a new release out today for this.

#6 would solve this for rendering messages that are invoked from the backend, but not solve the problem where purely front-end rendered messages still use a different template/rendering solution. The MR solves this. It's a bit weird to use drupalSettings for storing the template. feels like there should be a better way to deliver the template down.

Merge request made

bkosborne → created an issue.

Looks good

Merge request made

bkosborne → created an issue.

looks good to me

Given there are two different approaches here, it would be good to get thoughts from maintainers, then we can refine one of the approaches further and get it ready for review and merge. I updated the issue summary with the explanation of the two different approaches to make this easier to review.

Please, use the merge request instead of adding new patches. The patch in #32 seems like it is project-specific as it combines multiple patches. It's just going to cause confusion having it here in the thread. I'll update the merge request now.