undefined - Contrib.social

🇩🇪Germany Fabianx

While this is untested, here is the code that Grok created with some direction for VariationCache::getMultiple():

/**
 * Retrieves multiple cached items, handling redirects for each key set.
 *
 * @param array $items
 *   An array where each element is an array containing:
 *   - keys (array): An array of cache keys to retrieve entries for.
 *   - cacheability (CacheableDependencyInterface): Cache metadata for these keys.
 *
 * @return array
 *   An array where keys are the cache IDs, and values are the final cache items 
 *   or NULL if not found.
 */
public function getMultiple(array $items): array {
  $results = [];
  $cids_to_process = [];

  // Generate all cache ids for $items
  foreach ($items as $item) {
    [$keys, $cacheability] = $item;
    $cid = $this->createCacheIdFast($keys, $cacheability);
    $cids_to_process[$cid] = $keys;
  }

  // While there are still cache ids to process:
  while (!empty($cids_to_process)) {
    $current_cids = array_keys($cids_to_process);
    $fetched_items = $this->cacheBackend->getMultiple($current_cids);

    $new_cids_to_process = [];

    // Generate new list of cache ids to process further
    foreach ($cids_to_process as $cid => $keys) {
      $result = $fetched_items[$cid] ?? NULL;

      if ($result && $result->data instanceof CacheRedirect) {
        // If there's a redirect, generate a new CID to follow the redirect
        $new_cid = $this->createCacheIdFast($keys, $result->data);
        $new_cids_to_process[$new_cid] = $keys;
      } else {
        // If not redirected or no result, store the result directly
        $results[$cid] = $result;
      }
    }

    // Update $cids_to_process with new CIDs for the next iteration
    $cids_to_process = $new_cids_to_process;
  }

  return $results;
}

📌 | Drupal core | Render the navigation toolbar in a placeholder

🇩🇪Germany Fabianx

Whoever works on CachedPlaceholderStrategy here is how the raw architecture should look like (untested code):



// \Drupal::cache('render')
protected $cache;

// \Drupal::service('placeholder_strategy')
protected $placeholderStrategy;

// \Drupal::service('render_cache')
protected $renderCache;

function processPlaceholders($placeholders) {
  $cids = [];
  $new_placeholders = []; 

  foreach ($placeholders as $placeholder => $elements) {
    $cid = $this->renderCache->getCacheID($elements);
    if ($cid) {
      $cids[$cid] = $placeholder;
    }
  }

  $cached_items = $this->cache->getMultiple(array_keys($cids));

  foreach ($cached_items as $cid => $cache) {
    $elements = $cache->data;

    // Recursively process placeholders
    if (!empty($elements['#attached']['placeholders']) {
      $elements['#attached']['placeholders'] = $this->placeholderStrategy->processPlaceholders($elements['#attached']['placeholders']);
    }
    
    $placeholder = $cids[$cid];

    $new_placeholders[$placeholder] = $elements;
  }

  return $new_placeholders;
}

The reason for the recursion is to support the following scenario (part of ORIGINAL 2014 big pipe demo):

- There is a sleep(1) on the page in a cacheable block content, which sets max-age=0
- Then this makes the whole block uncacheable.
- As soon as you create a placeholder within the block for the current_time, then the block becomes cacheable again. You still want to create a placeholder for the block as it contains other content that is independent of the page.

With the CachedPlaceholderStrategy with above implementation:

- If the block is uncached then the whole block is streamed via big_pipe.
- If the blocked is cached then just the placeholder is streamed via big_pipe.

Without the recursive calling of the strategy, this would fail and the `current_time` would again make the whole page slow.

📌 | Drupal core | Render the navigation toolbar in a placeholder

🇩🇪Germany Fabianx

To comment myself:

The reason we have a ChainedPlaceholderStrategy in core by default is not only to support big_pipe, but because I envisioned that a lot of things that are not bound by URL would become placeholders in the first place.

As can be seen from the linked issue in #7 I had this whole idea of making it super easy to put e.g. a shopping cart placeholder on the page and replace it via JS, AJAX, etc.

And therefore the natural way to go about this is to use the chain:

- CachedPlaceholderStrategy
- BigPipePlaceholderStrategy
- SingleFlushPlaceholderStrategy

The reason is that the cached placeholder strategy can do a cache->getMultiple() [for anything that can be cached] while the SingleFlushPlaceholderStrategy is just rendering them one by one.

So even for just core this can save performance.

🐛 | Workspaces Extra | Prevent updating links that are tracked in a workspace

📌 | Workspaces Extra | Simplify getAssociatedRevisions() and getAssociatedInitialRevisions() from core

🇩🇪Germany Fabianx

amateescu → credited fabianx → .

🇩🇪Germany Fabianx

RTBM - Looks great to me!

📌 | Drupal CMS | Add performance testing

Comment 16 days ago →

🇩🇪Germany Fabianx

Looks great to me.

RTBC!

🇩🇪Germany Fabianx

xjm → credited fabianx → .

🇩🇪Germany Fabianx

poker10 → credited fabianx → .

📌 | Drupal core | Split the workspaces UI into a separate module

🐛 | Drupal core | Creating a published moderated entity in a workspace shouldn't make it published in Live

🇩🇪Germany Fabianx

RTBC

The approach makes sense to me. It feels a little bit weird to have workspace tests need to enable workspaces_ui but for integration tests the UI is obviously needed.

But one curious observation:

Why don’t I see removals of the new Hooks added to workspaces_ui to support toolbar, etc.

I had expected a big removal from workspaces.module, but there is nothing.

Maybe that is related to the hook_help problem?

🇩🇪Germany Fabianx

RTBC - fix is straightforward and the additional check ensures that we don’t regress in case someone else is setting the publication status for the live revision.

📌 | Drupal core | [ignore] Convert everything everywhere all at once

🇩🇪Germany Fabianx

I also reviewed the snippets of patches applied after the automatic conversion manually.

They all also look good to me.

The change record makes sense and even in D7 it was the right way to use module handler to invoke a single hook in another module.

Still RTBC

🐛 | Drupal core | Ordering of alter "extra hooks" is a gigantic mess

🇩🇪Germany Fabianx

RTBC + 1 from me - post merge

This implementation is much cleaner.

📌 | Drupal core | [ignore] Convert everything everywhere all at once

🇩🇪Germany Fabianx

RTBC - I did not look at every single line of code, but performed isolated spot checks and the conversion seems fully automatic to me.

I am also +1 to do this in one full sweep, so that we can get off .module files ASAP.

This MR also clearly shows that hooks are as easy to implement as before the conversion, so that’s a huge plus.

📌 | Drupal core | Allow for / implement simplified content workflow with workspaces

📌 | Drupal core | Workspace negotiators shouldn't be responsible for loading the negotiated workspace entity

🇩🇪Germany Fabianx

catch → credited fabianx → .

🐛 | Drupal driver for SQL Server and SQL Azure | Setting schema key is not supported

Comment 2 months ago →

🇩🇪Germany Fabianx

Now without debug code.

What this does is:

- Copy function __construct & setPrefix from pgsql driver
- Fix schema functions, where the prefixInfo schema is not used

🐛 | Drupal driver for SQL Server and SQL Azure | Setting schema key is not supported

Comment 2 months ago →

🇩🇪Germany Fabianx

Attaching patch for now.

🐛 | Drupal driver for SQL Server and SQL Azure | Setting schema key is not supported

Comment 2 months ago →

🇩🇪Germany Fabianx

fabianx → created an issue.

📌 | Drupal core | [DRAFT] [META] Drupal 7 policy on jQuery related vulnerabilities

Comment 3 months ago →

🇩🇪Germany Fabianx

mcdruid → credited Fabianx → .

🌱 | Drupal core | Automated performance testing for core

Comment 7 months ago →

🇩🇪Germany Fabianx

Fantastic work, catch!

🇩🇪Germany Fabianx

RTBC - looks great to me!

Issue summary could explain the approach with the token - for the most basic validation.

🐛 | Drupal core | Duplicate X-Content-Type-Options headers both with the value nosniff [D7]

🇩🇪Germany Fabianx

poker10 → credited Fabianx → .

📌 | Drupal core | OOP hooks using event dispatcher

🇩🇪Germany Fabianx

Patch looks great so far, but if it was me:

- We only allow one hook implementation per module right now (which is fine)

Why don't we just merge the collected listeners with the procedural when:

moduleHandler->invokeAll() collects the procedural implementations?

Then the whole module_implements_alter(), etc. would still work (without any changes or before/after!).

That would ensure for this FIRST iteration, we get OOP hooks via event listeners and auto-wired services, but moduleHandler really does not change except for discovering these implementations for the hook.

Before this was not possible due to how we did not have getListeners() so ordering became impossible, but now it's natural.

And we still throw an Exception() if a hook is defined more than once in the same way.

I feel - and I might be wrong - this would make for the most minimal patch.

📌 | Drupal core | OOP hooks using event dispatcher

🇩🇪Germany Fabianx

One thing I liked about the Hux-style approach was that you could just add a Hook attribute to any service and it was not limited to things in the hook namespace - you just tag the service as a 'hook_listener' and it works.

BUT this can and should be a follow-up.

I just want to ensure that this use case CAN still be made to work with the current architecture. (even if we then just later decide for or against it)

📌 | Drupal core | OOP hooks using event dispatcher

🇩🇪Germany Fabianx

I apologize if I am missing something, but:

#[Hook(hook: 'user_cancel')]

feels a little bit duplicated (or is that because of alter hooks being different?).

But I would prefer clearly:

#[Hook('user_cancel')]
#[AlterHook('module_implements')]

or

#[AlterHook('module_implements_alter')]

Therefore we get rid of the magic naming, but don't have: Hook(hook: ) duplicated.

But maybe it is necessary, then please disregard.

📌 | Drupal core | Add an API for allowing modules to mark their forms as workspace-safe

🇩🇪Germany Fabianx

Back to RTBC!

The new approach is fully BC compatible for layout_builder and makes it much easier for forms to mark themselves as safe by default / definition.

🌱 | Drupal core | Methods attributes to implement hooks (hux style)

🇩🇪Germany Fabianx

What Kris did here is kinda orthogonal to the approach and does not care where the events come from.

In essence adding support for //[@Hook(entity_insert)] style annotations to the existing patch is very simple:

- We know the service to call
- We know the method to call
- We just map that into the hook_entity_insert event (along side all other events)

The advantage is that:

- Full BC (and possibility to clearly deprecate module style invocations)
- Does not care if you use event listener to listen to the events or @Hook style invocation or .module file (for now)

We get rid of a lot of run-time stuff, which we cache anyway per hook and instead create for the .module things more like a registry of potential invocations.

📌 | Drupal core | Add an API for allowing modules to mark their forms as workspace-safe

🇩🇪Germany Fabianx

RTBC

My only concern was for existing code that sets workspace_safe = TRUE or FALSE, but the $form_state property always takes precedence, so this is just a way to set the default state of the form.

And it's very nice to use.

📌 | Drupal core | Mark Workspaces as a stable core module