🇺🇸United States @mrweiner

It turns out that revisions were on for our entity, but we never actually used them. As such we have no revisions to actually clean up and don't need this module. That said, I still think this would be a good approach to optimizing the performance if anybody is interested in tackling that.

Moving to NR as I cannot really test it due to https://www.drupal.org/project/revision_cleanup/issues/3487627#comment-1... ✨ Optimize for sites with large number of entities. Active . I see some other references to 'nid' so cannot be sure whether this works yet.

I'm trying to decide whether we need to roll our own solution to this, or if we can contribute a patch here. Will post an update if I have one.

mrweiner → created an issue.

Added additional config and field on the settings form to allow selection of any revisionable entity types on the site.

mrweiner → created an issue.

@nginex it looks like the module js input.value = iti.getNumber(); causes the dial code to be inserted to the input on blur, so we end up with the dial code next to the flag in the input. Did you figure out a way around this?

Forgot to mark NR

mrweiner → created an issue.

I don't think so, but can't say for sure. Probably want to test.

fixing typo in drush command

Last update was completely broken

Added update from https://www.drupal.org/project/stripe_api/issues/3417075 🐛 TypeError: Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch(): Argument #1 ($event) must be of type object, string given Needs review as well

Alright, draft MR is ready that adds the basic geolocation functionality. Leaving it a draft until I can add interfaces for method documentation.

• Adds configuration form/route + settings config and associated permission
• Adds CurrencyHelper service to determine available geo modules, get currency for the user's country, etc
• Updates CurrentCurrency to leverage geolocation if the setting is enabled
• Updates the cookie case to leverage new isUsableCurrency method instead of using loadByProperties. This will be slightly more performant since loadByProperties is not cached, whereas loadMultiple is.

I've added the geolocation setting as a module-level setting as opposed to a field-level setting. I'm not sure of a case where you'd want to use geolocation for one field's currency but not another. It would also add some complications due to interactions between field-level handling and the current global cookie handling.

Yup, that's what I'm thinking.

Updating the issue title :)

Yeah you're right, simplest option would be to use that commerceguys/intl currency map. Commerce Currency Resolver has a whole admin config form to be able to select which currencies apply to which countries. But that's probably overkill in practice, and overrides could be handled with a hook/event. Good call.

Yeah I'll do some experimentation. Do you think that an additional resolver is even needed for this? Seems like it could be handled in CurrenCurrency as an additional case. That's how CCR does it. There's a check for the modules in their config form, and then if the geo option is chosen then it's used in CurrentCurrency.

Or, I suppose, another option would be some way to leverage their CurrentCurrency service logic inside of commerce_currencies' CurrentCurrency logic, and then somehow bypass their resolver. Maybe this is just something I need to patch in on our end and not something you need to support.

This was a poorly thought out issue for me to have opened. I think I see what you're saying. From an implementation perspective, I don't think there is an "integration" to be built here -- commerce_currency_resolver would just need to add support for your multi-currency field in their resolver for the two to be compatible.

I think I'm just stuck in the middle where I like the approach and field over here, but I need the more robust geo-handling that they have over there.

Their geo-based currency determination actually isn't in a resolver, but just some service and config handling in their CurrentCurrency service. So that might turn this into a feature request ticket for geo-handling, which would basically be copy-pasting their code into this module. That just results in the same code essentially being maintained in two places, though, which isn't ideal but maybe unavoidable.

mrweiner → created an issue.

I was assuming that you were using hook_install here. My thought was just for e.g. if somebody installed commerce_currencies, and then subsequently installed commerce_shipping, the price field on shipments might not be updated as expected. But this might be moot depending on how commerce_currencies works.

Simpler option might be to just add additional submodules for each additional dependency. So one for each of tax, shipping, and promotions.

mrweiner → created an issue.

Thank you!

Typo

mrweiner → created an issue.

Seems to work as expected. This is actually my expected behavior for $settings['commerce_show_partner_banners'] = FALSE;.

Changing the title to reflect the purpose of the patch. Policy concerns remain in the issue description.

MR 39 fixes Event class change

mrweiner → created an issue.

Sorry I've been away from the issue queue for a while. Sounds related to https://www.drupal.org/project/media_entity_remote_image/issues/3117255 → which should have been fixed before. Will take a look!

Closing out. If anybody runs into this, feel free to reopen.

Not sure what you mean. When I visit https://www.drupal.org/project/media_entity_image → , I see a different project as expected. Happy to look again if you can give more info.

@jsacksick re: #80, I can confirm that the patch in #77 fixes the issue for me on D10.3.0.

Confirming that MR !25 fixes the issue for me.

mrweiner → created an issue.

Re-rolled #17 against latest dev.

updated MR to apply to 2.0.x

mrweiner → changed the visibility of the branch 3249576-ability-to-disable to active.

mrweiner → changed the visibility of the branch 3249576-ability-to-disable to hidden.

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

Re-rolled #234 for latest 10.2.6.

There was one conflict:

CONFLICT (modify/delete): core/modules/system/tests/src/Functional/Theme/TwigDebugMarkupTest.php deleted in 51703edde0 (Applying patch 231 from issue 2118743) and modified in HEAD.  Version HEAD of core/modules/system/tests/src/Functional/Theme/TwigDebugMarkupTest.php left in tree

But, just left the file in per HEAD.

mrweiner → created an issue.

I don't have full knowledge of how this module works, so I'm unable to robustly test the automated changes in this patch. That said, they all look basic enough.

I've created a new 2.x release in order to apply these patches and bump compatibility to include D10. If you need to upgrade to D10, you should update to 2.x-dev.

Bumping this to 2.x branch

mrweiner → created an issue.

mrweiner → created an issue.

Alright, so I actually did the same thing as above but just in buildForm() instead of in the ajax callback. That worked for getting the form to work as expected in general.

Since it's an existing entity form, I needed to override it with my own implementation to be able to change the buildForm behavior, like so

function my_module_entity_type_alter(array &$entity_types) {
  $entity_types['node']->setFormClass('default', \Drupal\my_module\Form\NodeForm::class);
  $entity_types['node']->setFormClass('edit', \Drupal\my_module\Form\NodeForm::class);
}

Then I needed to add custom validate callback to grab the value from the form and set it as the value.

function my_form_alter(&$form, $form_state_interface) {
    $ajax_wrapper_id = 'date-ajax-wrapper';
    $form['my_field']['widget']['#prefix'] = "<div id=$ajax_wrapper_id>";
    $form['my_field']['widget']['#suffix'] = '</div>';

    // Add an AJAX callback to the date field.
    $form['field_dates']['widget'][0]['value']['#ajax'] = [
      'callback' => 'my_callback',
      'event' => 'change',
      'wrapper' => $ajax_wrapper_id,
    ];

    // Make sure to unshift so it runs before the default validation,
    // or submission will fail due to null value for the submitted field. 
    array_unshift($form['#validate'], 'my_callback');

}

function my_callback(&$form, FormStateInterface $form_state) {
    $vals = $form_state->getUserInput()['my_field'];
    if ($vals) {
      // Logic to get/modify your values as needed
      $form_state->setValue('my_field', $vals);
    }
  }

Just a note that I was able to solve this issue in my node_form_alter with the following:

function my_form_alter(&$form, $form_state_interface) {
    $ajax_wrapper_id = 'date-ajax-wrapper';
    $form['#prefix'] = "<div id=$ajax_wrapper_id>";
    $form['#suffix'] = '</div>';

    // Add an AJAX callback to the date field.
    $form['field_dates']['widget'][0]['value']['#ajax'] = [
      'callback' => 'my_callback',
      'event' => 'change',
      'wrapper' => $ajax_wrapper_id,
    ];

}

function my_callback() {
    $form['my_field']['widget']['#options'] = ['a', 'b'];
    foreach (Element::children($form['my_field']['widget']) as $i) {
      unset($form['my_field']['widget'][$i]);
    }
    Checkboxes::processCheckboxes($form['my_field']['widget'], $form_state, $form);
    return $form;
}

No need to fuss with ajax responses and whatnot. Just wrap the entire form, return the entire form, and ensure the checkbox field is totally updated. The problem is that updating '#options' alone is not enough. All of the children checkbox elements need to be replaced as well.

Was just coming here to post the same.

mrweiner → created an issue.

mrweiner → created an issue.

#5 works for me

mrweiner → created an issue.

For anybody who needs a quick and dirty fix for this, I'm handling it in hook_preprocess_field() with:

function cc_gin_preprocess_field__node__body__article(&$variables) {
  // Assuming $text is the HTML content you're preprocessing
  $text = $variables["items"][0]["content"]["#text"];

  // Create a new DOMDocument and load the HTML content
  $dom = new DOMDocument();
  @$dom->loadHTML(mb_convert_encoding($text, 'HTML-ENTITIES', 'UTF-8'), LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD);

  $links = $dom->getElementsByTagName('a');

  foreach ($links as $link) {
    // Check if the link is an internal node link
    if ($link->hasAttribute('href') && str_starts_with($link->getAttribute('href'), '/node/')) {
      // Extract node ID
      $path = $link->getAttribute('href');

      // Fetch the alias for the node
      $alias = \Drupal::service('path_alias.manager')->getAliasByPath($path);

      // Replace the href attribute with the alias
      $link->setAttribute('href', $alias);
    }
  }

  // Save the updated HTML
  $variables["items"][0]["content"]["#text"] = $dom->saveHTML();
}

Note that this was generated by chatgpt, so I'm not sure that all of the args in $dom->loadHTML() are needed, but it seems to do the trick for me.

mrweiner → created an issue.

Rad, thanks!

That committed D7 patch references http://stackoverflow.com/questions/32224697/mailchimp-api-v3-0-change-su.... One of the solutions there mentions that you can update the contact email via API. Here is a basic way to do this for users specifically.

function mymodule_user_presave(UserInterface $user): void {
  if (!$user->isNew()
    && $user->getEmail() !== $user->original->getEmail()) {
    $original_email = $user->original->getEmail();
    $new_email = $user->getEmail();

    $lists = mailchimp_get_lists();
    foreach ($lists as $list_id => $list) {
      $info = mailchimp_get_memberinfo($list_id, $original_email);

      if (empty((array) $info)) {
        continue;
      }

      $tokens = [
        'list_id' => $list_id,
        'subscriber_hash' => md5(strtolower($original_email)),
      ];

      $parameters = [
        'email_address' => $new_email,
      ];

      /* @var \Mailchimp\MailchimpLists $mc_lists */
      $mc_lists = mailchimp_get_api_object('MailchimpLists');
      $mc_lists->request('PUT', '/lists/{list_id}/members/{subscriber_hash}', $tokens, $parameters);
    }
  }
}

It loops over the lists looking for contacts matching the user's original ID. If one is found, it makes a PUT request to update that contact's email. Since this is done in hook_entity_presave(), it completes before functions like mailchimp_lists_process_subscribe_form_choices(), which will then update the contact instead of adding a new one.

I feel like there must be a cleaner way to do this in the MC module proper, but it's been a long couple of weeks and I just need this working.

Or is it not a problem that some of the event instances may be out of sync with the series configuration?

Exactly this. The eventseries represents a music lesson schedule, and the instances represent music lessons. Teachers attach things like lesson notes, teaching materials media to share with students, etc, onto the instances, so I need this information to persist regardless of the overall schedule change. The initial date is also valuable in itself.

I'm sure there are a variety of use cases where folks might need a particular instance to stick around even if it's out of sync with the series.

Sorry anybody who has hit this.

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

Probably worth marking NR for visibility.

Here's a patch that I created for my own use. The message I added in the event that instances were skipped probably isn't ideal for all cases. It might be nice to provide some way for the user to modify the message/provide their own message. Maybe another alter hook specifically for the message?

Patch doesn't provide tests.

mrweiner → created an issue.

I suspect that I'll need this functionality, so leaving a note for myself or anybody else who decides they need to hit this -- I don't think it'll be too involved. The instance creation logic is in RecurringEventsFieldTypeInterface::calculateInstances(), implemented in the Drupal\recurring_events\Plugin\Field\FieldType classes. Likely just need to add the integer field (like suggested by @the_glitch) and then use that to modify the generated instances.

Alternatively, if folks want to implement this themselves, there is hook_recurring_events_event_instances_pre_create_alter(). If you add the integer field yourself, you could pull the value in this hook to filter out the instances before they are created.

mrweiner → created an issue.

mrweiner → created an issue.

Anybody who stumbles across this, a way to mimic entity access in the context of views is to actually just check the entity access in views_post_execute(), as in

function cc_views_post_execute(ViewExecutable $view) {
  $removed_results = 0;

  foreach ($view->result as $key => $row) {
    $entity = $row->_entity;
    $access = $entity->access('view', \Drupal::currentUser(), TRUE);

    if ($access->isForbidden()) {
      unset($view->result[$key]);
      $removed_results++;
    }
  }

  // Update the total rows.
  $view->total_rows -= $removed_results;
  $view->pager->updatePageInfo();
}

This isn't as robust as what's proposed in this post, but is a very simple way to get a similar result out of views. You'll likely need to do some cache handling for this as well.

Also +1 to

Maybe we want to add "My media" vs. "All media" tabs to the default media library view and let sites that want to restrict access to the "All media" display do so via the usual Views access settings (e.g. to make them role based, or whatever).

Two tabs -- maybe with associated "view media overview" permissions -- would be great.

Looking through this issue, I wonder whether we can take inspiration from the permissions that Drupal Commerce sets up, e.g. View any Order, View orders in own store, and View own orders.

Per #11:

As media items, by default, everyone would be able to view them. The point of "view X" permissions in core is to limit who can see something at all. We do not want to confuse things by having a permission called "View own media items" that actually means "see only your own media items when trying to insert one into something else."

I agree with this, unless we are in fact defining a global permission. Would it be problematic to introduce a "View own media items" permission to handle this case more generally? I'd wager that this would handle most of the use cases that folks are trying to address when landing on this issue.

This actually may duplicate the discussion in https://www.drupal.org/project/cer/issues/2998138 ✨ Could support Remove field name prefix module? Needs work

Sorry for the mess of branches and patches. The MR should be the correct updates and the correct fork/branch. The MR also includes asort() on the options, which isn't present in the patch. Not sure whether this needs tests or not. Maybe a module setting for whether to include base fields?

For some reason I'm having trouble pushing to the issue fork, but here's a patch to make base fields work. The module has a preg_match in it's logic to only allow "field_" fields. The patch just removes this and then adds a nullable check for target_bundles, since it may not be present on a base field at all.

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

either use hook_module_implements_alter(), to get your hook_form_alter to act after field_group.

This actually is not possible without modifying module weights, because field_group_module_implements_alter is run after custom implementations. So even if you do implement it, field group's will run later, causing field_group_form_alter to run last.

Thanks for reporting this, Andres! Was finally able to fix this up. :)

Typo in title

mrweiner → created an issue.

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