🇺🇸United States @neclimdul

I'm not sure I understand why we're not using standard methods for attaching things to the footer.

Not sure of a common real case but I was able recreate this just now like this.

1. Install Drupal site.
2. Ensure registration is allowed.
3. Log out and visit the registration url with a complex url fragment. e.g. https://d10.lndo.site/en/user/register/#badfragment/broken

xjm → credited neclimdul → .

Took a stab at laying down some ground work for the suggestions. This actually is pretty cool because a lot of the hardcoded things that made it impossible to really theme or extend the password strength region are tied to the strength indicator so focusing on the suggestions allows it to be more extensible which can sort of be seen in the latest updates.

I don't claim this is a "good" implementation but it hopefully keeps things moving forward.

Nice!

Yeah, probably not measurable from a wall clock perspective with current PHP performance hence the title. However, it removes one assignment, one function call to implode, and the admittedly trivial string concats implode is doing from every twig_render. This means on a stock Drupal install it saves 54 function calls and assignments on an uncached request to the frontpage so its at least measurable from that metric. Since most sites are probably more complex then that and as you said its actually a bit cleaner it seemed worth while.

Makes sense. I remember browser instrumentation generally not working so I've never run it though so I've got some questions for people that do use it:

1. Are there possibly additional problems with caching with how manual adds adds code? Specifically, is the "header" being added unique to the request? I see the settings tag but should this be a lazy callback or have additional tags to avoid page/render caches?
2. In the post update, should it take the Drupal version into account and migrate to manual instead of migrating to a auto that behaves like manual with a not immediately obvious warning? Or maybe it should warn the user during the update? Unsure.

reroll in MR

Quick reroll on MR

Its not a "problem" per say. It seems the way the code gets run now the output isn't being captured but is instead ending up being output.

From memory, { "command":"buildAll"} is the response from solr saying "I recieved the buildAll command from you and ran it."

I actually agree and disable phpinfo in production systems entirely. I think in previous discussions some people wanted phpinfo to be available in production so people could check modules and other php configuration which is what lead to the current approach and why I suggested the more target hardening. But also not trying to blow the scope, just wanted to be mindful of an impact.

Core has some protections in place to avoid these sorts of sites leaking critical information after https://www.drupal.org/sa-core-2023-004 →

But if we're making this a core feature we'll be encouraging people to do even more, a good thing imho. But since this means it expands the places and levels of experience using the feature, it does have me wondering if when adding the core feature, we should also harden it to it warns people if they have their system configured in a way that could be unsafe. Maybe a requirement check that detects environment variables would be available through core's phpinfo and this DB auto-configuration is being used it throws a warning like we do with settings directory permissions or allow_updates.

created

Test should be fixed.

I think the concerns might be a bit overstated. Actual problems will trigger real exceptions that get logged. This just avoids warnings that shouldn't be triggered. If I remember correctly they're warnings triggered by the underlying libraries so they don't actually follow PHP's documentation for the method.

got some tests in place that identify some of the pending issues. not passing because we've got some hard problems to solves still.

re: 📌 Simple fixes for words with prefix of 'de' or 're' RTBC

dequeue is the correct technical term for removing an item from a queue. I'm not sure if unqueue is actually a word but what I can find it has a different meaning.

reference:
- https://en.wikipedia.org/wiki/Queue_(abstract_data_type)
- https://computersciencewiki.org/index.php/Queue
- https://www.programiz.com/dsa/queue
- etc

These changes where just accidentally dropped from a re-roll. Patch to re-introduce them.

neclimdul → created an issue.

Got a test that's failing against core:

faddb2be4808:/app/core$ yarn test:nightwatch --test tests/Drupal/Nightwatch/Tests/a11yTestPager.js 

[Tests/A11y Test Pager] Test Suite
────────────────────────────────────────────────────────────────────
ℹ Connected to chromedriver on port 9515 (93ms).
  Using: chrome (106.0.5249.103) on LINUX.

  ℹ Loaded url http://d10.lndo.site in 259ms
  ℹ Loaded url http://d10.lndo.site/user/reset/1/1714417934/bnXISZHcCy0Vaj7yqQkhlbKvsb2pb2R8YKembDx4hMY/login
 in 384ms
  ℹ Loaded url http://d10.lndo.site/admin/modules in 381ms
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]> was visible after 528 milliseconds.
  ✔ Element <#system-modules-confirm-form> was present after 514 milliseconds.
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]:disabled> was present after 8 milliseconds.
  ℹ Loaded url http://d10.lndo.site/user/logout/confirm in 168ms

  Running pager ellipsis is accessible:
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  ℹ Loaded url http://d10.lndo.site/pager-test/ellipsis in 81ms
  ✔ Testing if element <.pager__item--ellipsis> is visible (20ms)
  ✔ Passed [ok]: aXe rule: aria-allowed-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-allowed-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-conditional-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-deprecated-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-prohibited-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-required-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-roles (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr-value (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: color-contrast (2 elements checked)
  ✔ Passed [ok]: aXe rule: duplicate-id-aria (1 elements checked)
  ✔ Passed [ok]: aXe rule: empty-heading (1 elements checked)
  ✔ Passed [ok]: aXe rule: landmark-unique (1 elements checked)
  ✔ Passed [ok]: aXe rule: link-name (3 elements checked)
  ✔ Passed [ok]: aXe rule: listitem (4 elements checked)
  ✔ Passed [ok]: aXe rule: presentation-role-conflict (1 elements checked)
  ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

  FAILED: 1 assertions failed and  17 passed (385ms)

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

  ️TEST FAILURE (7.154s):  
   - 1 assertions failed; 20 passed

   ✖ 1) Tests/a11yTestPager

   – pager ellipsis is accessible (385ms)

   → ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

 Wrote HTML report file to: /app/core/reports/nightwatch/nightwatch-html-report/index.html

Rebased and pushed test to the MR.

Looked at this some. The asset rendering rendering pipeline is a mess and fights this at every step so I'm not even sure is going to work without updates to core. Some many things change in the sort depending on if its optimized or not. there's all the grouping. And each component relies on the logic of the other steps and assumes only internal and external files are possible.

Also found this that's probably related in some way. 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work

Going to keep hacking on this but its pretty complicated and at the very least it can't really happen as late as the renderers.

You can't run the entire test suite with coverage, it will definitely die. When I ran coverage reports every commit during the 8.x release cycle, I'd only run the unit tests because it doesn't really make sense to run the other tests. They don't provide @covers, and they don't cover specific pieces of code but features. Also, because like you'd have a million tests populating "coverage" for the bootstrap but none of them testing anything.

We actually do the filtering in core its just been renamed in xdebug 3 for obvious reasons.

  <coverage>
    <include>
      <directory>./includes</directory>
      <directory>./lib</directory>
      <directory>./modules</directory>
      <directory>../modules</directory>
      <directory>../sites</directory>
    </include>
    <exclude>
      <directory>./modules/*/src/Tests</directory>
      <directory>./modules/*/tests</directory>
      <directory>../modules/*/src/Tests</directory>
      <directory>../modules/*/tests</directory>
      <directory>../modules/*/*/src/Tests</directory>
      <directory>../modules/*/*/tests</directory>
      <directory suffix=".api.php">./lib/**</directory>
      <directory suffix=".api.php">./modules/**</directory>
      <directory suffix=".api.php">../modules/**</directory>
    </exclude>
</coverage>

Aside from all that, in general I wouldn't surprised if even unit tests for core had problems. I stopped running coverage reports because they'd constantly get broken because core wasn't running them so they don't get reviews and commits would constantly break them in weird ways. That's why I'm here! :-D

oh that looks promising. That does look a bit off but it's just their collection so shouldn't be a big deal.

Still agreed core needs a better setup in core but this should solve the immediate issue once its released.

Yeah, that looks right. I was suggesting we just add that environment variable to the default container config or the CI config since its unlikely anyone would want any other xdebug mode in CI.

on the php directives, looks like you where passing '-dzend_extension=xdebug -dxdebug.mode=coverage' to phpunit script but you'll need to pass them to php like php -dzend_extension=xdebug -dxdebug.mode=coverage ./vendor/bin/phpunit.

I suspect the Xdebug problem was just it running in the wrong mode. We could probably add the environment variable or set the default config to always run in coverage mode. I couldn't find the failed test though to confirm this.

Is there a advantage to adding pcov? Xdebug has always worked well enough for me.

Thanks! That's really helpful, I think I can take a first pass based on this.

On the last point, I think I understand I could test the pager block and that makes sense and seems like the right approach. I'm not sure that fully addresses my concern though. The ellipse could be failing but the test pass if the environment doesn't have enough content or configuration changed in a way as so the ellipse isn't rendered. Should it target the ellipse directly?

xjm → credited neclimdul → .

I didn't notice it in my initial review but with further testing I'm seeing some weirdness in javascript sorting. I think this might be causing problems with dependencies since they aren't being considered in the resort.

agreed

neclimdul → created an issue.

Opened a merge request that applies a similar patch to both the CSS and JS rendering.

I'm also not sure hot the aggregation/optimization interacts with this but based on some basic testing the aggregation did not seem to be affected by this sorting. That's to say, by using weight to place an inline element between two aggregated scripts, they where split into two aggregation files served before and after the inline javascript.

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

Feels like maybe the whole train of issues down that link are caused by this and the complexity it causes. Probably need to fix the underlying issue instead of chasing our tail trying to fix every edge case caused by it.

Ok, probably makes sense to have some support for the privacy stuff that is documented. "functionality_storage" "personalization_storage" "security_storage"

As far as doing this in the current advanced settings, I don't see a way atm. Are you suggesting adding something to the advanced settings to support it?

For reference, this came up implementing the suggested configuration for Osano.

https://docs.osano.com/hc/en-us/articles/22469433444372-Google-Consent-M...

neclimdul → created an issue.

Replacing a extension sounds pretty dangerous.

The current approach of calling \Drupal::service is functional but seems like a _huge_ performance hit going to the container ever template render.

Does anyone know why the deprecation compiler code was written into the extension? It seems like it might have just been convenient.

If it was important, maybe we can move it to a dedicated "extension" that's not really an extension that just provides this helper?

If the approach isn't important, maybe we can just inline the check. Something like this maybe:

    $compiler->write("isset(\$context['deprecations']) && array_map(");
    $compiler->indent();
    $compiler->write("fn (\$name) =>");
    $compiler->indent();
    $compiler->write(
      "isset(\$context['deprecations'][\$name])",
      "&& \array_key_exists(\$name, \$context)",
      "&& @trigger_error(\$context['deprecations'][\$name], E_USER_DEPRECATED),"
    );
    $compiler->outdent();
    $compiler->subcompile($usedNamesNode);
    $compiler->outdent();
    $compiler->raw(");");

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

This doesn't sound like a core bug. The addition of style sheets(and javascript) is all handled through libraries so if a webform component needs progress but isn't getting it then it just needs to add it to the list of required libraries in its render array or one of the other methods drupal provides.

https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j... →

Fair enough.

However, I don't think I've really got a firm answer on the correct approach and I had to modify the proposed resolution so before I go diving into tests I'm going to move this back to NR to get some review on the approach.

Took a first pass at the proposal in the IS. Don't know if that was correct or not.

Couple changes:

1. Since this is structured, I used aria-details instead of aria-described by per the note on the aria-describedby mdn documentation https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...
2. Added aria-live to the dynamically changing region

I also had to mangle things a bit to push an id onto the suggestions but I think this might make things a bit better by not replacing the entire wrapper div. This might allow the aria-live to actually work.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to active.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to hidden.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to hidden.

seems like with gitlab we could start moving toward just using phpunit directly? it would make this pretty trivial and could better integrate with all the other features gitlab provides

Don't know. We've been running this to great effect for close to 4 years now to and haven't seen a problem during that period and xmlsitemap never shows up on our cron metrics. Would be happy to help move this forward.

I haven't had any troubles with paragraphs but I suspect paragraphs is fighting with the mousedown behavior to work around core so there would need to be follow up I guess.

Yeah, using try/catch as a conditional doesn't feel great. I considered moving the regex to a constant we could use but that felt weird to so open to suggestions.

https://drupal.slack.com/archives/C1BMUQ9U6/p1709912605839939

Needs test but approach could use some review.

Realized my concerns about optimizing this for existing conditions is probably not a big deal. Core is passing urls and that's probably the common use case. fromUserInput is probably the expected behavior the rest of the time.

That said, we can avoid cloning is we just created the object so I've included that in the merge.

Needs tests but the existing twig extension test needs a lot of scaffolding for each test so I will come back to it.

for what its worth, this has likely triggered a bunch of noisy output from cron 🐛 buildAll command output durring cron runs. Active

Yeah, that seems pretty important since my understanding is that's the only place we'd be supporting this this right? Managing dependencies in the active store or exports sounds like an impossible task that would largely undo the consistency promises of config.

As a workaround, you _can_ use the internal: prefix. For example link('text', 'internal:#anchor'). We should still address this though because this wouldn't be obvious to your average developer. Or experienced developer since I needed to have catch point it out and I think he had to dig through Url.php to find it 🤣

neclimdul → created an issue.

despite several requests for comment, its still not clear why we need a special case for "ng-"

If we do need to special case it, hard coding it like this in its current location has a smell and suggests to me we haven't succeeded in making this "consistent" if such a special case for a framework is required.

conflicts with 📌 Replace deprecated String.prototype.substr() with String.prototype.substring() Fixed . rerolled in a merge request. not sure what a test would look like so just the reroll atm.

oh... sucks we don't have a standards compliant YAML parser anymore.

We worry about function calls in a ton of other circumstances that might be hot code paths. Not sure why this would be different so follow up MR posted with the approach Catch mentioned.

As far as the tests, they are clearly not testing what they think they are. The only reason to test serialization on them is to assert they're correctly interacting with dependency serialization which they're bypassing so there's definitely a bug. If we're not fixing it here, then we need a follow up.

I wish we could help surface this sort of trap to developers because its obviously easy to fall in. But like I said, I guess we can't build test suite logic into things outside the test suite. That sort of code is what we where removing from this trait to start with its just unfortunate.

That makes sense as an optimization.

I guess I'm worried that the tests we where changing where explicitly testing container aware serialization without a container. That's kinda a red flag to me but maybe we can't force better testing with logic outside the test suite.

Oh and it removed all the other changes. So, I think the failure was probably important. Wasn't it saying "hey, you're not testing what you think you're testing. There's not actually a container available."

At some point this changed from using \Drupal::container to Drupal::service() in a loop. Could that potentially add a bunch of extra function calls to a wakeup?

Updated IS

Since the changes should only affect changing I don't think this counts as an "API Change" so captured that in the IS.

I'm not sure that the sync is required but i don't see a problem with it so here's a combined version with the sync and also the more robust helper method.

"I would agree with the overall goal that Recipes are currently progressing how config works in drupal core, providing tools 'more like' Features in Drupal7 ecosystem."

Not sure what that means. For my use cases there are better versions of everything in the D8+ ecosystem so I'm just not the target audience.

"To mention the 'want's of this issue that I would ask around that recipes isn't handling (that i know of!). is ENV based config (config_splits using its 'partial configs') dev/uat/prod codes. Otherwise the goal of having better code side tools and UI's are part of the Recipes outlines.."

"Yes, I agree that ENV based config should be easier. Currently, it makes more sense to use settings.php to override config, because then I don't have to manage multiple partial configs."

I use environment driven config _very_ heavily and its pretty straightforward today. Keys module is great if a little rough in the UI. Its designed for "secrets" but the model is allowing the mapping from stores to any config value so mapping from environment to any config value is easy and very powerful and has so far met every need I've had for varying settings per environment with the exception of things like enabling development features locally which still make since to manage with splits.

I'm not sure. Looking through the summary, it seems like this was about taking a fresh look at config and taking the underlying API to the next level. The initiative you linked seems to be about providing better starter configuration/profiles/recipes. I could see how updating the config API could help that but those sound like different things and the issues tagged to CMI 2.0 don't seem particularly tied to that initiatives goals.

neclimdul → created an issue.

neclimdul → created an issue.

Made a library people can use to "test" this in their projects.

https://www.drupal.org/project/cache_remember →
https://packagist.org/packages/drupal/cache_remember

I'm fairly happy with the current version of the methods with one exception and that's the fact its still annoying to set the expiry since we set a timestamp instead of an offset. You need to pull the time service and calculate it instead of having a central way of doing this.

Not sure if there's a good way to address this while maintaining consistency with other Drupal API's but I'm open to suggestions and we can test them out.

Do we need to hang this on a ckeditor feature with an indeterminate timeline? Seems like we could provide this as a feature with the caveat and support the new feature after ckeditor supports. That would provide some immediate functionality for people that can use it as is but also highlight the feature request to more users.

With that in mind though, we should probably default this off and make sure administrators opt in and understand what they're opting in to.

A quick test of the merge request, this seems to work pretty well as a user. Very nice!

Is the proposed resolution that was agreed to the static list?

Moving parent since we dropped the deprecation path.

Thought this had stalled but looks like a lot of great work was happening I just wasn't watching the right issues! Thanks!

"as long as we're using the correct type of merge pipeline." welp...

Rebased and address the feedback from alex.

Unfortunately this doesn't work. It was really broken and the ID attachement didn't seem to work at all. I fixed that but it only works for simple elements. When trying to attach to groups like radios or checkboxes, the ID in displayErrorMessages is the collection, but the ID in setElementErrorsFromFormState is the ID of the specific input and these don't match.

For example testing on umami search,

edit-type--error-message

but the element is

edit-type-article

referencing

edit-type-article--error-message

Definitely needs a lot of tests.

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

From: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predef...

The HEAD SHA of the target branch of the merge request. The variable is empty in merge request pipelines. The SHA is present only in merged results pipelines.

Sounds good. Seems like this will work as long as we're using the correct type of merge pipeline. That also explains why we can get rid of the fetch so great news!

Bug in core's spellcheck step is blocking tests. 🐛 Spell-checking job fails with "couldn't find remote ref refs/heads/11.x" when 11.x branch does not exist in fork Active

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

sorry for the noise. Forcing a 11.x branch to the issue fork to work around a bug in the core gitlab config.

The patch seems to be working great. Note about how we can make it better in the review.

neclimdul → changed the visibility of the branch 11.x to hidden.

neclimdul → changed the visibility of the branch 11.x to hidden.

neclimdul → changed the visibility of the branch 11.x to hidden.

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

There's a lot to unpack in this merge request but wanted to point related to these changes the current spellcheck logic isn't great and probably not behaving as intended. The way it checks the remote branch actually checks against the issue fork which can relies on both the branch existing and actually matching the branch we're merging against. This only kinda works on recent forks but on _old_ forks it can be completely broken and fail entirely.

This probably needs its own issue unless this is close to being resolved.

benjifisher → credited neclimdul → .

This seems like it is in a good place. The mitigation is pretty straight forward.

This seriously affects the ability to administer sites without a documented work around so bumping the priority.

The code is pretty confusing and its not clear to me how it fixes the problem but i don't seen anything broken.

I might be high-jacking this issue but it accurately describes the problem I've been tracking for a while now.

Quick tangent:

(queue workers starting to spit errors due to missing / expected properties.

This actually sounds like 🐛 Use item_id instead of qid for Queue item Needs review which actually happens when queue items are being processed. The lists being inconsistent should trigger claimItem to start returning a lot of false values which clients should interpret as a failed queue fetch and skip processing.

Theory of the problem

So I the problem comes down to how the garbage collection gets handled. GC is not atomic and can happen across multiple clients at the same time.

To set a scenario that might be able to reproduce this lets imagine a queue talking to an API and that API having some downtime. All those requests are failing. If the worker is smart enough to throw a RequeueException, those items would get immediately but if they crash the exception gets logged and the item is left to expire.

Later the items expire and GC kicks in. Lets imagine 2 workers trigger at close to the same time(they're hot looping claim item so this is likely) and get basically the same list of claimed items. They then loop through checking the leases. They're both going to see them same expired items and remove the claim and importantly _both_ push the item back onto the available items. This is a list not a set so we now have duplicate entries in the list which starts causing problems.

The best case is that 2 workers will be able to claim the duplicate item ids and work through them without a problem.

More likely later workers will either fail triggering the the same process or be unable to claim the item because it no longer exists.

Now the kicker that's been causing me headaches. Once a worker is able to process them item, claimItem will never be able to get the actual item because hget failures just get ignored meaning an expiring never gets set leaving the entry to be GC forever but also feeding back into the race condition.

Proposal

Making some things like the item list sets that can't have duplicates would probably be handy but I'm not sure how that would get implemented, if there are reasons for using a list over a set etc. So a couple of fixes can probably solve this.

Also, having two parallel lists of the available items is probably bound to lead to these sort of inconsistencies and this theory around race conditions might not be the only cause. So some sort of GC to keep it consistent is probably a good idea.

1. Implement a firewall so GC can't push items back into the available list if they no longer exist.
2. Have claim item clear inconsistent entries?
3. Implement some sort of cleanup like the exist patch that keeps the list consistent? The current approach looks a bit expensive for busy queues, should probably use exist instead of hget, and it might introduce its own race conditions so I'm on the fence for this.

Took a stab at automatically detecting if the program updating resource should be used. Best I can tell this is the right approach but the documentation is very confusing. Let me know if this help.

So my understanding:

1. Documentation is bad. pushToMarketoUsingPOST requires a program name but doesn't document this requirement. It in fact documents the opposite programName (string, optional),
2. With formid, program name is bypassed. Maybe not a huge deal but a secondary bug.
3. Without formid, webform calls update lead. Without a program this fails because of #1.

The solution is then to provide a push and sync method and choose the correct one based on the program logic?

Marketo really made this so simple!

That's troubling but I'm not sure how to move forward with a fix without something I can recreate :(

This was fixed in the marketo library. Looks like the user found the fix.

I'm having trouble finding any documentation on what the deprecation is and what the replacement is so can't move forward with a fix atm.

We're in a good place. Thanks bot!

neclimdul → created an issue.

That's true, but we have a d10 release. We didn't but we do. Feel free to post some patches though, if there is some activity we can reconsider it.

reroll. Conflicted with 🐛 InfoParser returns an empty array if passed a non-existing file Fixed

quick fix for style problems.

I liked that idea and needed the idea for a project outside Drupal so I wrote this.

https://gitlab.com/neclimdul/phpunit-exceptions

Its a trait that works mostly the same but has some additional functionality and the assertions are accomplished a bit different.

Let me know if this would work for Drupal or if there are changes that would help.

@alayham Yeah, the suggestion all along has been to avoid any localization and compare number/byte sizes directly.

https://git.drupalcode.org/project/drupal/-/merge_requests/670/diffs#not...

This was only in needs work because it doesn't have tests which is still true.

Skimming the discussion, I'm not sure which result was giving multiple seconds. Do you mean the _really_ small results that are in scientific notation with E-5?

I'm curious how this is able to generate hashes of files that are lazily generated. Some more digging to understand the patch I guess unless someone has a hint.

Marking NW and adding needs tests back because there are not tests in the patch. We can continue discussing performance impact outside that fact.