🇺🇸United States @mariacha1

The patch supplied will try to subscribe (or unsubscribe) users to mailchimp not only when they make a change to their user account, but also any time the user entity (or whatever entity the field is on) is saved, including programmatically. This can lead to accidental unsubscribes.
See https://www.drupal.org/project/mailchimp/issues/2775335 → .

I'm still looking for someone to provide more information about exactly what the field settings are, because although I do fully believe it's a problem, I haven't been able to reproduce it.

Can you tell me a bit more about how you'd set up a subscription field to make it mandatory? What are your field settings?

As due dilligence I tried out a variation of field settings like this:

Required field: True
Enable Interest Groups: True
Hide Subscribe Checkbox for new subscribers: True
Default value: Unset

This created a field on the registration form that had a bunch of unchecked interests and showed no "Subscribe" checkbox.

As long as I selected an interest, I was subscribed. If I didn't select any interests, I wasn't, which is the behavior I'd expect.

I also tried out using the functionality that hides all the fields and then set a default value, like this:

Required field: True
Enable Interest Groups: True
Hide Subscribe Checkbox for new subscribers: True
Hide Interest Groups: True
Default value:
- Subscribe checkbox: True (and I tested Falseo here too)
- Automatic interests: Red, Green

I thought if the "Subscribe" checkbox was unchecked in the backend, perhaps the widget would not notice that some interests were set, but that seemed to subscribe people as well.

MR opened with fix and tests. Diff for composer available at https://git.drupalcode.org/project/mailchimp/-/merge_requests/86.diff

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

It turned out our code was depending on the "default_value" of each facet to display a "clear" button and apply a class, so here's a version of the last patch that preserves that value per-facet, along with an interdiff between comment 4&5.

For me, this throws errors if you have multiple ajax calls on the page. I'm providing an alternative that only clears out facets once they're sent to the autocomplete function, so that you don't get errors about autocomplete_settings[data_id] being empty.

I'm working on this ticket as part of the Drupal2024 First Time Contributor workshop and am mentoring a few people who will say hello below!

We're going to be reviewing this patch.

mariacha1 → created an issue.

Great -- and this is merged into the 8.x-1.x and 8.x-2.x branch.

Actually, I noticed that if you are already a MC user and sign up in Drupal without checking any interests, you get unsubscribed. That's not supposed to happen, and would be blocked if we were calling mailchimp_lists_process_subscribe_form_choices from places OTHER than on the postSave. But on postSave, you already have an entity id, so isNew() returns FALSE.

Fix applied to the 2.x branch.

Merge request is here:
https://git.drupalcode.org/project/mailchimp/-/merge_requests/82

Noting that I believe the problem is from this PR:
https://git.drupalcode.org/project/mailchimp/-/commit/5cba54651db7cfb3a9...

from https://www.drupal.org/project/mailchimp/issues/3231414 🐛 Subscription box is never checked Fixed

which deleted the "Subscription form" field widget.

This change keeps you from being able to sign up for a newsletter when registering (says "Invalid email configuration.") or when editing the entity (just shows you what you're signed up for), which feels like an unintended regression.

I think it's important we add that FieldWidget back before the next release, so I'm upping the Priority.

@stephenplatz I'm re-opening with "Needs work" so you can submit a patch or merge request here -- I think your proposal to add !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\field\FieldStorageConfigInterface') to the`field_permissions_jsonapi_entity_field_filter_access` is great.

This isn't a robust-enough solution to be a patch, but here's the ImportProcessor I'm using in my custom module if it's useful for other people:

<?php

declare(strict_types=1);

namespace Drupal\mymodule\Plugin\EntityShareClient\Processor;

use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Plugin\PluginFormInterface;
use Drupal\entity_share_client\ImportProcessor\ImportProcessorPluginBase;
use Drupal\entity_share_client\RuntimeImportContext;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Import book structure.
 *
 * @todo This can be deprecated if
 * https://www.drupal.org/project/entity_share/issues/3441672 is fixed.
 *
 * @ImportProcessor(
 *   id = "metatag_importer",
 *   label = @Translation("Use Server Canonical link"),
 *   description = @Translation("Get the Canonical url from the Metatag attribute on the Server."),
 *   stages = {
 *     "prepare_importable_entity_data" = 20,
 *   },
 *   locked = false,
 * )
 */
class MetatagImporter extends ImportProcessorPluginBase implements PluginFormInterface {

  /**
   * The entity type manager.
   *
   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
   */
  protected $entityFieldManager;

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    $instance = parent::create($container, $configuration, $plugin_id, $plugin_definition);
    $instance->entityFieldManager = $container->get('entity_field.manager');
    return $instance;
  }

  /**
   * {@inheritdoc}
   */
  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    return $form;
  }

  /**
   * {@inheritdoc}
   */
  public function prepareImportableEntityData(RuntimeImportContext $runtime_import_context, array &$entity_json_data): void {
    if (isset($entity_json_data['attributes']['metatag'])) {
      // Find out if there's a Metatag type field on the node.
      $parsed_type = explode('--', $entity_json_data['type']);
      $entity_type_id = $parsed_type[0];
      $entity_bundle = $parsed_type[1];
      $entity_fields = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $entity_bundle);
      $metatag_field = NULL;

      foreach ($entity_fields as $id => $field) {
        if ($field->getType() === 'metatag') {
          $metatag_field = $id;
        }
      }
      if ($metatag_field && $entity_json_data['attributes'][$metatag_field] === NULL) {
        foreach ($entity_json_data['attributes']['metatag'] as $value) {
          if (isset($value['attributes']['rel']) &&  $value['attributes']['rel'] === 'canonical') {
            $entity_json_data['attributes'][$metatag_field] = [
              'value' => [
                'canonical_url' => $value['attributes']['href'],
              ],
            ];
          }
        }
      }
    }
  }

}

This would be enabled as a plugin called "Use Server Canonical link" at the import config level under `/admin/config/services/entity_share/import_config`.

mariacha1 → created an issue.

Here's the interdiff.

I'm attaching the interdiff between 42 and 45 (ignoring the .info stuff). It's worth noting that the merge request is slightly different based on the need in https://www.drupal.org/project/field_permissions/issues/2881776#comment-... ✨ Implement field permissions per-bundle (field instance) Needs work .

It would be good to know if we are still trying to make the per-bundle option something a user can choose to configure. In the current patches all field permissions are per-bundle and none are per-entity.

I'm taking steps to move this issue onto the 8.x-2.x branch, which currently matches the 8.x-1.x branch. This per-bundle permissions feature will be the major difference between branches.

I can confirm that changing the Text Format via the dropdown makes the ckeditor5 toolbar on 10.2.2. I'm attaching a gif of the behavior.

I've also shown in that gif that resizing the page or toggling the menu to the sidebar fixes the issue, so I tried to debug a little but the js is beyond me. The best I could understand is that the "drupalViewportOffsetChange" event is triggered by both of those things (resizing, swapping the admin toolbar orientation), but it must not be triggered when you swap editors. I also found that the top of the CKEditor toolbar is set using "editor.ui.viewportOffset" which is supposed to be at 79 but ends up being at 0 when you swap editors. So maybe something isn't being initialized correctly during editor swap?

Aha, it looks like https://www.drupal.org/project/drupal/issues/3348132 🐛 Dynamically instantiated CKEditor 5 instance's toolbar occludes Drupal's toolbar if it has focus and the form is part of paragraph Active is what you're describing. So I'll post/follow there and hope that takes care of things. Thanks for the hint!

mariacha1 → created an issue.

I agree, I don't think it's a problem in D10. I haven't seen it as a problem anywhere since 2019, so probably safe to close.

Here's a quick reroll. I D10 I was seeing errors about there being no value for "$element['#array_parents']" on regular node forms for any select or radio fields added directly to the top-level of the form. This updated patch just wraps the foreach that loops through array_parents in an if statement.

Here's a reroll of the patch that works with the 3.0 branch.

Last patch still had a duplicate function in it.

Rerolling the patch from #67 without the double reference to $entityFieldManager;

This feels more like an entirely new behavior, and a new checkbox would have better backwards compatibility too, in case people are somehow counting on there not being a connection between users and redhen contacts when the user is created programmatically.

Maybe a new setting "Connect RedHen contacts to new users": "Will attempt to connect Drupal users to RedHen contacts by matching email addresses whenever a user is created."

And, as you've noted, the "registration_link" aka "Link to existing contacts" is not used at all, so it could be taken off the settings form in the "Registration" section, and the new checkbox could replace it (but not be in the "Registration" field group).

Here's a patch that does this. It could be pretty easily updated to also pull in a Subscription to an Order using reverse lookup. I heavily copied the relationship plugin from Webform Views, which, as I said in the description, would allow us to do the same thing with orders, if desired.

mariacha1 → created an issue.

Ooops, bug in the code to get the value of next_renewal. Fixed!

Here's a patch for above.

mariacha1 → created an issue.

Here are a few more changes to make this new page easier to get to.

Change 1: Link the email telling you a payment was rejected to this new page to fix it.
Change 2: Add an action button to the order page so if your order is in "needs payment" state, you can fix the payment right from the order page.

Here's a patch for the warnings.

mariacha1 → created an issue.

I'm just leaving a quick note on how I solved the problem in case it can help others.

Since commerce_recurring uses "advancedqueue" with an option to set the processor to either "daemon" (meaning run by drush or drupal console) or "cron" I just added some lines to my settings.php file to say "only run commerce_recurring on cron on live". With that I'm able to test out recurring charges on my local or dev databases using drush advancedqueue:queue:process commerce_recurring (and have it run against my testing sandbox for credit cards, handled also through settings.php) while still having the live site run on cron. Here's the code in my settings.php file (example is for Pantheon):

// Protect ourselves from a cron run attempting to renew subscriptions.
$config['advancedqueue.advancedqueue_queue.commerce_recurring']['processor'] = 'daemon';

if (defined('PANTHEON_ENVIRONMENT') && (PANTHEON_ENVIRONMENT == 'live')) {
  $config['advancedqueue.advancedqueue_queue.commerce_recurring']['processor'] = 'cron';
}

I definitely don't fully grasp what changes would be needed to this PR to get it to work with https://www.drupal.org/project/commerce/issues/3047103 → , but this small addition to #55 does allow you to add a payment method to an order that's in the "needs_payment" state when you visit, for example, /user/[UserId]/orders/[OrderId]/payment-retry.

I'm kind of hard-coding the workflow, though, and that's probably not right. If anyone can point me in the proper direction, I'm happy to make more changes.

Here's a patch in case this seems reasonable...

mariacha1 → created an issue.

Oops, bad patch contained nothing!

Here's that patch

mariacha1 → created an issue.

Shoot, I didn't find that open issue! That's the same issue as this one in my opinion, although the patches there only deal with pushing. @AaronBauman I'm fine with closing this as a duplicate and moving patching over to there, if it's ok to also work on pulls in that issue too?

mariacha1 → created an issue.

mariacha1 → created an issue.

This patch works, but only if you've got "Send mail to default mail" turned on. I'm attaching an updated version of the patch that also works if you're not using that option as well.

Aha, thank you for the info!

mariacha1 → created an issue.

I bet you're right! Although I then got more complicated and created a base entity type field, but I didn't see it creating any tokens either. Here's what my Plugin docblock looks like:

@ComputedField(
 *   id = "my_test_field",
 *   label = @Translation("My test field"),
 *   field_type = "string",
 *   no_ui = TRUE,
 *   attach = {
 *     "scope" = "base",
 *     "field_name" = "my_test_field",
 *     "entity_types" = {
 *       "taxonomy_term" = {},
 *     },
 *   },
 * )

I ended up defining my own token for this, but was pretty surprised that it didn't seem to work out of the box, so wanted to be sure I wasn't missing something simple.

The value of the field, when checked, is true, not "match", which is preventing the code from running.

    handling:
      match: true

I'm including a patch to fix that.

mariacha1 → created an issue.

Alright, initial test cobbled together from copy/pasting Node and User tests. This is almost certainly buggy -- I haven't run it locally yet -- but I can come back and clean it up or someone else can if I get pulled away for too long.

Patch is applied to the latest 8.x-1.x branch, and I've opened a followup ticket at https://www.drupal.org/project/field_permissions/issues/3374151 📌 Tests: field_permissions_entity_field_access field_definition is a base field Needs work to test the hook for non-FieldConfig elements in the future.

mariacha1 → created an issue.

Aha, yep, this patch both fixes the problem and is more correct than mine. Nice! I tested on a clean branch this morning, and will merge shortly. Thanks for the teamwork everyone!

100% -- I don't even think it'd be too hard to write that test. I'll merge this the second someone who's experiencing this gets me a thumbs up, since I don't have a clean install running locally just now.

Here's that patch. Let me know if it solves the issue!

Ahh, good catch. More the fool me for reviewing on a site with my own bunch of patches applied.

I think this is a fine solution for the problem for now. I'd like base fields to be useable in the future, but that's a different problem.

The most fool-proof solution is to just check to see if $field_definition implements \Drupal\Core\Field\FieldConfigInterface, since that's what fieldGetPermissionType expects.

So this:

  if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition, '\Drupal\Core\Field\FieldConfigInterface')) {
    return AccessResult::neutral();
  }

Just on the off chance there's a non-base-field that is also not a config field (this is something a module like https://www.drupal.org/project/entity → may define for BundleFieldDefinition)

Ultimately we'll fix that difference so that fieldGetPermissionType handles non-FieldConfigInterface field definitions better, but there are other issues open for that, and this solution should fix the immediate need.

I'll send up a patch for this in a bit.

Patch over at https://www.drupal.org/files/issues/2023-07-10/2881776-39.patch → should fix it. If you're using the previous patch (32), you should be able to roll that one instead.

I'm attaching a patch that is compatible with the latest dev release and contains a fix to this patch that fails if you're looking at the field in a view (which does not provide a bundle, core issue: https://www.drupal.org/project/drupal/issues/2898635 🐛 [PP-1] bundleFieldDefinitions() are not added in EntityViewsData Needs work ). This should fix https://www.drupal.org/project/field_permissions/issues/3373500 🐛 TypeError: Drupal\field_permissions\FieldPermissionsService::fieldGetPermissionType(): Argument #1 ($field) must be of type Drupal\field\FieldStorageConfigInterface Fixed if people are seeing it.

I'll try to get this merged into the new 2.x branch this week or next so we can stop with all the patching.

Agreed, I'm going to assume you've got a patch applied that's conflicting with the latest push to 8.x-1.x. I had this trouble related to a patch in https://www.drupal.org/node/2881776 → . I'll update that issue with an updated patch.

Cool, merging!

@jhedstrom I'm glad you're as confused as I am!

Tests don't fail, and it does look like that's why this was added in the first place.

My only note is that the "hook_jsonapi_entity_field_filter_access" hook below also checks against "display" and that should probably be "view" as well.

Otherwise this feels pretty good to me.

greggles → credited mariacha1 → .

Actually, messed that one up. Here's the real patch.

Here's the patch

mariacha1 → created an issue.

Here's a reroll of the patch for 9.5.9.

Here's that patch

mariacha1 → created an issue.

In our case, we are using Search API and have a facet for a taxonomy that has hundreds of entries. Because the facet becomes invalidated by changing any of the terms within that taxonomy, the search results page has cache tags for basically every term on the site.

Here's that bare-minimum patch

mariacha1 → created an issue.

This latest patch works great for me, and the code makes sense!

+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -8,14 +8,15 @@
-use Drupal\Core\StringTranslation\StringTranslationTrait;

This removal of the "use" statement is throwing an error for me, and it doesn't seem relevant to the rest of the code changes in this file. Maybe this was an accident?

Adding it back to 245 fixes it for me.

Ok, at this point the only test failing is related to the d7 -> d8+ field migration, which checks to make sure that the third-party settings on the FieldStorage match between the d7 and d8+ config. Since this patch changes where the third-party settings for this module live, pushing them onto the field config (field.field) level, it's a valid failure.

That migration process would need to be rewritten to move the settings onto the bundle level, and then this test would need to be updated.

Alternatively, if this is going onto an 8.x-2.x branch, I suppose we could say that branch doesn't support migration from d7, and drop both the migration and drop the test. If we find ourselves beyond end of life for D7 and this hasn't been merged into a 2.x branch, that is the easy solution.

I'm applying the patch from https://www.drupal.org/project/field_permissions/issues/2881776#comment-... ✨ Implement field permissions per-bundle (field instance) Needs work in a new merge request open here:

https://git.drupalcode.org/project/field_permissions/-/merge_requests/10...

It does two things.

1. Applies to the latest version of the 8.x-1.x branch.
2. Updates the referenced class to be Drupal\Core\Field\FieldConfigInterface instead of Drupal\field\FieldConfigInterface. The first locks us from altering permissions on fields defined using \Drupal\Core\Field\Entity\BaseFieldOverride (like the promoted field) which was technically possible with the entity-wide version of the module (although to get it to work with the UI, you needed something like https://www.drupal.org/project/base_field_override_ui → )

Let's see how tests go!

smustgrave → credited mariacha1 → .

Previous tests passed and I've added a new one -- if that one passes I'll set to "Needs review".

I think the real issue here is that the original patch assumed that Published was the only way that access to a page would be managed. Of course, there are a ton of ways view access can be manipulated in Drupal, including Content Moderation, different permission levels, and access_alters. The patch at https://git.drupalcode.org/project/drupal/-/merge_requests/3279.diff accommodates those instead of just relying on the "status" column in the database. It should also not cause tests to fail.

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

Current problem seems to be that my recieved hash never matches my generated hash, so the files are never created.

+++ b/core/modules/system/src/Controller/AssetControllerBase.php
@@ -0,0 +1,261 @@
+    if ($received_hash == $generated_hash) {

Ok, working through the code, I see #269 doesn't apply the filesystem changes to the new lazy classes, so here's the patch with that applied as well. This patch also does the following things:

• Group the use statements and removes initial backslash
• Removes a docblock for `deleteAll()` that duplicates its parent verbatim
• Rearranges the getAll and deleteAll functions so they're easier to compare to the deprecated classes.