🇺🇸United States @bkosborne

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.

Hi Jakob, I noticed there's a new Acquia-provided configset available called "(Latest) Drupal 9/10 - Search API Solr 4.3.2 - Solr 8 - [drupal-4.3.2-solr-8.x-0] - v1.0

Should I follow this process:

1. Update from Search API Solr 4.2.x to 4.3.x
2. Edit the existing Solr index in Acquia Search and change its config set to the new one
3. reindex? presumably?

Do I need to completely delete the search core (index?) or is just changing the config set suitable?

Thank you

this isn't so much a "works as designed" as it is a real feature request to allow controlling this in the UI. Work is progressing towards this in a separate issue.

Sounds good

Hi, sure! Looks like you have a good amount of experience contributing and maintaining other projects. Glad to have the help.

Okay, pushed an MR. However, the test needs a bit of work. The test today isn't testing much. It's passing 3 tags to the form to save, but in reality, only the first one is being processed by Drupal. This seems to be a problem with how the form request is being simulated with Request::create(), which is passing the form params as query strings and they aren't being processed into the request object in the same way that Drupal does with Request::createFromGlobals().

In any case, if that were be resolved, the test modifications should pass.

bkosborne → created an issue.

It can certainly be challenging to come up with descriptive text for images!

I think that the descriptive text could be improved though to better help the users understand what a decorative image is, rather than just dumping them on the web accessibility page. I'll leave this open for that.

they get alarmed like their image will not show to some people.

They should get alarmed that their image won't show up to some people, because it's true. Blind users that navigate the web using a screen reader will not "see" the image if it doesn't have alternative text.

The alt text description field from core indicates the purpose of the alt text "Short description of the image used by screen readers and displayed when the image is not loaded. This is important for accessibility."

This module's entire purpose is to force users to stop and think about what it means to avoid entering alt text. The only valid reason for NOT having alt text on an image is if it's decorative.

I'm open to modifying the description to explain a bit more about what it means to mark an image as decorative.

But ... if we used your suggestion of "Check to skip adding alt text", then there's really no reason to use this module at all, and you might as well uninstall it. Just let your users keep the alt text field blank.

I'm certainly open to changing this to be more descriptive. Do you have any proposals? The image is essentially hidden from screen readers. To my understanding, screen reader software will skip over an image that has empty alt text as if it does not exist.

bkosborne → created an issue.

Thanks for the review. I'll see about extending that base class!

Another thing I want to try and resolve here is this scenario:

1. Authenticated user clicks a link to /cas?destination=/some/page
2. User is denied access to /cas since they're already logged in
3. Our subscriber intercepts the exception and redirects the user to the homepage
4. The /some/page destination is lost

We have at least one site that's using this workflow for links that this patch breaks.

Made some minor changes to the issue summary and reviewed the newer commits from yesterday. This looks good to me. This seems like it was a lot of work. I learned a lot about enums from reviewing the code! Such a better solution than what we used before!

This was a huge merge request, I reviewed it at a high level.

Looks good to me.

I just saw this change record. Awesome work!!

Created an MR that adds an advanced setting to configure the value for cookie_domain for gtag. This doesn't affect sites using Tag Manager tags. I went with the simplest approach for now, but I could see this being expanded to support all the other cookie config options as well.

This also makes another seemingly unrelated change. Previously, when gtag was initialized for each tag ID, only the primary tag ID (the first one) would get the set of additional configuration options passed in. This only included the custom dimensions and metrics. I'm not sure why this was done, but it seems to me that all tags should get the set of dimensions and metrics passed in. What makes the first tag so special? This allowed me to pass in the cookie domain config to all tags.

#3 is focusing on google tag manager. While this module supports google tag manager (GTM), not all sites using this module are using GTM. You can use Google Analytics directly and use this module and it will load the google tag script (gtag). This script supports setting the cookie domain, and we need the ability to influence that configuration like the old Google Analytics module did.

I'm in the same boat as vinmassaro. gtag's default cookie domain setting will result in a huge number of cookies accumulating if you run an organization with hundreds or thousands of sub-domains.

This looks good. Agreed merging to a new major branch is the right move. Thank you for doing all this.

Hmm... are you sure? That whole code block is wrapped in an if statement that only executes when the "ckeditor" module is installed (CKEditor 4). So if you don't have CKE4 installed, then it shouldn't block anything when attempting to install this module.

Ugh, sorry, forgot to add you as a contributor to that commit before I made it. My email was screwed up on it too so... no one gets credit. Sigh.

Ah, yeah. Thanks! Committed and released 3.0.0-beta1

I don't see how that's possible. $elements is an array typed property on the function. If it were null, PHP should have thrown a different error when _asset_injector_add_element_libraries was invoked. What version of Asset Injector are you using, and what other patches (if any) do you have applied?

catch - you linked to this issue instead of what you intended to

I haven't tested it with workflows, but I think it wouldn't work with content moderation.

We ended up doing this:

/**
 * Implements hook_entity_field_access().
 */
function HOOK_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
  $accessResult = AccessResult::neutral();
  if ($operation === "edit" && $items) {
    $entity = $items->getEntity();
    // Prevent users from unpublishing a node that's set their homepage.
    if ($field_definition->getName() === 'status' && $entity instanceof NodeInterface && $items->getEntity()->status->value) {
      $isHomePage = /* function call to check if the entity is the homepage */
      $accessResult = AccessResult::forbiddenIf($isHomePage, 'The homepage may not be unpublished.');
    }

  return $accessResult;
}

This removes the ability to unpublish the page from the UI, like when editing a node (the publish checkbox is removed) or using bulk actions on the main content list view.

johnv → credited bkosborne → .

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

bkosborne → changed the visibility of the branch 3455489-sanitized-email-addresses to hidden.

I'm very confused. There are two branches, 8.x-2.x and 8.x-2.x-release. The former is using SHA2 and the latter has been set to use MD5 to calculate the hash for 6 years. Is the 8.x-2.x branch abandoned?