🇺🇸United States @mrweiner

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.

Possibly related to 🐛 Login link destination URL parameter doesn't work if site installed in sub directory Active

Attaching patch to address the issue. There's a comment in the patch noting that this probably isn't the ideal place to do this, but I couldn't get it working elsewhere. Maybe somebody else can work out a better implementation.

mrweiner → created an issue.

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

mrweiner → created an issue.

@smustgrave issue summary needn't be updated because no actual decisions have been made regarding how this should be addressed, unfortunately.

mrweiner → created an issue.

@aiglesiasn closing this as a duplicate. I've made some progress on this in the other ticket.

Alright, this is mostly working except for the drawer invalidation. The problem here is that the drawer invalidates all of its slots' cache entries by invalidating its own cache tag, but the drupal_static has no knowledge of this tag, nor can it. Not sure of the best way to deal with this at the moment.

Chatting on slack but adding some notes in here just for future reference.

/**
* @var object
*/
protected $staticTagsCache;

We don't want to keep this "temp" cache on the slot object, because the same data needs to be accessible from every slot instance with the same ID. As in

$slot_1 = $manager->openSlot('my_module', 'some_api', 'id');
$slot_2 = $manager->openSlot('my_module', 'some_api', 'id');
$slot_1->setCache('some_data');
$slot_1->getCacheData == $slot_2->getCacheData(); // <-- this needs to be true, but will not be if you're returning data from a property on a specific slot.

$this->staticTagsCache = &drupal_static(__FUNCTION__);

This is problematic for a similar reason. Magic __FUNCTION__ is scoped to the class instance, and not the cache ID. We'd need to do like &drupal_static($cache_id); to ensure that the data is associated with the proper cache entry and not the name of the doSetCache function.

    if (empty($this->staticTagsCache)) {
      $this->staticTagsCache = (object) [ 'id' => $this->id, 'data' => $data, 'expire' => $expire, 'tags' => $tags];
    }

We don't want to check whether it's empty. This should just mimic the regular setter in this method, setting the static value with the same data as the cache entry. We also need to make sure that the static variable is set such that it has the same properties as on $this->getCache() (data on the data property, etc). It needs to mimic a cache entry.

    $cache_item = $this->cache->get($this->id, $allow_invalid);
    if (!$cache_item) {
      if (!empty($this->staticTagsCache)) {
        $cache_item = $this->staticTagsCache;
      }
    }
    return $cache_item ?: NULL;

If we always set a value in doSetCache, then these conditionals need to be handled differently, so something like.

$static_cached = drupal_static($this->id);
$cache_item = $this->cache->get($this->id, $allow_invalid);

return $static_cached ?: $cache_item ?: NULL;

Only snag is $allow_invalid -- I don't know how this would be handled by the static implementation.

mrweiner → created an issue.

mrweiner → created an issue.

mrweiner → created an issue.

Maybe the "real" solution to this would be to update the Recurly module itself to use the v3 api. Likely not a small undertaking, and y'all could weigh the benefits of such a move. I'd be happy to help with this update if we want to go that route.

@eojthebrave I see the confusion. Back when #37 was written, https://github.com/hfischberg/v2Recurly-client was created for me by a recurly support member, and it seemed that the v2 branch would be sunset. Their versioning is confusing (for me, at least), because the api version differs from the SDK version. The latest v2 api on https://recurly.com/developers/api-v2/ is 2.29, which is the same latest release as 3 years ago, while the recent 2.x release on github is 2.14.1.

To compound my confusion, they also have 3.x and 4.x releases that both seem to relate to the v3 api, without documentation as to what the difference is.

You're probably right that it doesn't make sense for this module to use that fork given that it's way out of date and 2.x continues to receive updates. I guess maybe my team is just in the unfortunate situation of needing to run both v2 and v3 concurrently. In an ideal world, we could have an up-to-date v2 package separate from the v3 package, but I don't know that there's a good way to do that unless one of us wanted to maintain a fork to track 2.x.

This doesn't really offer a solution, but I think this essentially summarizes where things stand.

reroll

re-roll

mrweiner → created an issue.

Hate to RTBC my own patch so much later, but any chance of getting this committed? We are still relying on a fork of this module that's very outdated at this point. Would love to kill it and bring us back up to date.

In case anybody else stumbles upon this, I implemented the following to use a specific menu to populate the links in the toolbar.

function module_preprocess_links__toolbar_user(&$variables) {
  $menu_tree_service = \Drupal::service('menu.link_tree');
  $menu_name = 'user-toolbar';
  $parameters = new MenuTreeParameters();
  $parameters->setActiveTrail([$menu_name]);

  $main_tree_loaded = $menu_tree_service->load($menu_name, $parameters);
  $main_tree_built = $menu_tree_service->build($main_tree_loaded);

  // Note that this uses laravel collections, but you could do the same with a standard array_map
  $links = collect($main_tree_built['#items'])
    ->map(function (array $item) use ($variables) {
      return [
        'link' => [
          '#type' => 'link',
          '#title' => t($item['title']),
          '#url' => $item['url'],
        ],
        'text' => t($item['title']),
        'attributes' => $item['attributes'],
      ];
    });
  $variables['links'] = $links->toArray();
}

@roberttabigue the RTBC status should be used when a particular patch has been reviewed and fix the issue. Since you're saying the issue has been fixed without needing the patch, RTBC is not the correct status. I'm going to mark this as closed instead. Feel free to revert if anybody feels the need to do so.

I'd assume this may need tests but seems to have fixed the issue for me.

Looks like the update from the patch was added/remedied in 📌 Paypal integration Fixed

Merging this is in for further remote development

mrweiner → created an issue.

Re: #79, having worked extensively with SFC, I can offer some insights

We have an intermediate format that IDEs don't understand, and that will not likely try to support (imagine working without autocompletes).

This is true, with caveats. It may be possible to configure IDEs such that they can understand these files, depending on how they are structured. See discussion in https://www.drupal.org/project/sfc/issues/3202242 💬 Document IDE support Active , with approaches for both vscode and phpstorm. My experience with phpstorm is that it works, but there are some pains. You can get it all working with PHP autocompletion, but there are issues with code autoformatting that jetbrains doesn't want to work on. It's likely that if we did implement SFCs in core, we might need to maintain IDE plugins or provide documentation on how to get things working if the front matter suggestions can be made to work out of the box.

We have a format that our browsers won't point to in debuggers (no source maps from what the browser is running to the typescript embedded in the SFC).

Yes and no. SFC module generates JS files behind the scenes which are easily inspectible. The SFC itself isn't referenced, but the JS files are generated per component anyway so it's trivial to make the associations during debugging.
We have a format that is text only (which excludes binary files like images, fonts,...).
True. I don't think it's the end of the world to create a directory when things need to be associated with an SFC, but I can see why one wouldn't consider it ideal.
We have a format that code quality tools will not support (which leads us to turn them off).
Yes, this can be a headache.

We have a format that is unique to Drupal that surprises both new-comers and old-timeys.

Unique in implementation, but not in principle. As was mentioned, vue leverages SFCs. Just because something isn't as familiar doesn't mean it's a wrong approach per se.

We have a format that we have to document and maintain the documentation for.

True, but we will also need to document and maintain everything for SDCs as well.
We have a format that gets more confusing as a compo­nent grows (imagine scrolling back & forth vs. having 3 editor tabs open).
The people arguing for SFCs will disagree with this. I 100% prefer working with SFCs ,both in drupal and in vue, even for longer components. If I want to have two tabs open for markup and styles, I can just open the file in two tabs, no big deal. Like the SFC maintainer said in the parent issue "Less files === more good, which is SFC's unofficial slogan, probably."

Re: #85

One other issue that I think SFC will create is making it harder to have variations of components. For example, in Umami we have a view mode for card and one for card alt and one for card common. We then have a css file for card.css and card-alt.css and card-common.css. The latter two css files are very small. If we had SFC then I'm not sure how we'd be adding these small variation CSS files, I'm sure we'd figure it out but I wouldn't like the solution being we use single file components except for variations where we use single file components for the main components and single directory components for the variations.

Not exactly sure what the issue is, here. Why couldn't a variation just include its own css? Those very small stylesheets would be inlined like any others.

OK, but that's not what https://git.drupalcode.org/project/drupal/-/merge_requests/3657/diffs is doing with the Umami cards - which are just a title, image and 'read more' link.

I agree that the approach it takes is problematic in that the components themselves are tightly coupled to entity templates. Ideally any variables would be passed in (classes, title_prefix, etc) and content could potentially be variably positioned via slots (or {% blocks }%). But, to be fair, that issue has been postponed until everything is sussed out here.

A general note -- when working with SFC, I found it useful to always use twig embed and include with the only keyword (i.e. {% include 'card' with { title: 'A title' } only %} to ensure that the component isn't polluted by variables that aren't explicitly passed in (i.e. props). Not sure how this concept fits into the vision for SDCs.

@longwave re: single file components, maybe check out https://www.drupal.org/project/sfc → . I've implemented it on a project and absolutely love it (probably also because I love vuejs). It parses the styles and js from the sfc file and converts them into a library behind the scenes (along with other niceties).

Although the CSS belongs to the component, as far as I understand it there is no scoping of CSS going on here. That means you still have to be careful about reusing class names and writing your CSS so it doesn't affect other components. If somehow we could automatically scope CSS to the component only, that would be a really big win here for me.

True -- some sort of css scoping would be a big win. We use BEM to structure all of our sfc components to avoid these types of collisions.

how would they be done so they can be re-used in different contexts? i.e. the media browser card shouldn't be media-specific, or possibly not even entity-specific?

This is kind of the entire point of leveraging components. A "card" component and any variants would be provided somewhere other than media -- maybe a core_components module, or similar -- and then implemented wherever it's needed. If media wanted to re-implement or extend the base card component then it would be free to do so.

ust seem to have a 1:1 mapping (3:3 in the Umami case) of template to component - the CSS and JS is rearranged (which I totally get as a good reason for doing this), but it also adds an extra layer of indirection in the original template and some metadata that appears to be effectively repeating hook_theme. I think it would help to have a good example, taken from core, that demonstrates why the extra indirection is actually useful and how it can reduce repetition somewhere (and if it can't, then I am not sure I see the benefit in adding this as-is).

The only other way it makes sense in my head would be to expose the component as a view mode, which would still require some form of pseudo variables that can be hot-swapped within the fields UI.

Another point for the approach that SFC takes, which is that you can designate a component as a template override, like so

<?php
$definition['overrides'] = [
  'node__article',
];

I think a feature like this would be very nice to have (maybe a need-to-have) if a component system were to land in core. This also relates to @longwave's concern:

Nice, good catch. Going to mark as NR to trigger tests/get some visibility.

Update proposed resolution per MR implementation.

Updating the issue title and description since the scope is larger than just the permission noted originally.

mrweiner → created an issue.

Alright, given that tests are passing, and reiterating that I think #57 perfectly sums up why this approach is okay, I'm marking this RTBC.

Alright, I've created an MR that includes the patch from #56 and a fix for the documentation issue noted in #57. After reading through things again today, I think that @AaronMcHale was spot on in #57, so I've left the permission name as-is instead of changing it to list_builder_permission or whatever else. I agree that followups are probably the place to handle additional core entity types. If tests pass then I will mark this RTBC in the hopes that we can move things forward.

Also want to bump this. The semantics arguments around viewing one vs many entities was a complete departure from the actual goal of this issue: to provide a permission for granting access to the route that displays the view provided by the entity's list_builder handler. This really had nothing to do with json api or general entity access.

@mglaman had a perfect solution for all of this in #29

list_builder_permission would mitigate the semantics issue around the route being "collection" and permissions being named "overview". The use case we're working against is the list builder, not an API resource with a collection of entities.

If I remember, I'll try to roll a new patch this week.

mrweiner → created an issue.