🇺🇸United States @danflanagan8

Aha! There is a way to get runtime cacheability for argument-related plugins! I knew I had done something that worked before. That was in the contrib space for 🐛 Cache tags not added to View Fixed . Luckily, my patch has a great comment that leads us to the core issue: #2853592: Cacheability metadata can't be set from within argument default handlers. →

That technique seems really weird, but it could be used in the absence of a better api.

fjgarlin → credited danflanagan8 → .

It tuns out this fix doesn't really do anything because Views do not calculate cacheability of argument_default plugins at runtime. D'oh!

The same goes for argument and argument_validator plugins as well. The fun will continue in #3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page →

On that security issue, @lendude had some nice comments that I want to put here:

The main issue I see here is getting the correct tags and contexts if a term (or the same seems to go for any entity) doesn't validate. We can add the term tag but the actual validation/access check is done using \Drupal\views\Plugin\views\argument_validator\Entity::validateEntity, which only returns a boolean and not a \Drupal\Core\Access\AccessResult. So we lose all the relevant cache information from the access check.
Ah. Just read @acbramley IS on https://www.drupal.org/project/drupal/issues/3091671 → and that seems to have the same conclusion :)

So adding the Term tags will solve the problem outlined here, but I don't think it would solve all the issues

Not too familiar with the Context definitions, but that also doesn't seem to contain anything relevant to access checks right?

Also, another potential issue we need to consider, if we do this on the argument_validator plugin level, the cache information will only bubble up to the config since all cache metadata is stored in the View config, so information about specific term IDs/access will not be available at that point.

I replied with:

Thanks, @Lendude. I think it's #7 that makes this really hard :(

In fact, the "fix" I put in for https://www.drupal.org/project/drupal/issues/3427374 🐛 taxonomy_tid ViewsArgumentDefault plugin doesn't add cache tags Fixed is at once completely accurate and completely worthless since the cacheability is only calculated when the View is saved.

It seems like we need to calculate the argument/argument_default/argument_validator cacheability at runtime somewhere.

I ran into a similar situation recently. It can technically result in information disclosure, but the security team decided this is an issue we can handle in public. Here's how I reported the issue to security (issue 180326):

Views entity argument_validator does not properly cache access, leading to the possibility of information disclosure.

There's already a public issue from a few years ago that mentions this: https://www.drupal.org/project/drupal/issues/3091671 →

Here's an example of how this can result in a user seeing a View they shouldn't see. This is a weak example in that none of the content itself is off limits, but it demonstrates the main issue well enough.

1. Do a standard install
2. Create an article that has one published tag.
3. Create a page View of articles that shows titles at /validator-test. Add a contextual filter for " Content: Tags (field_tags)" with the validator configured as "Taxonomy term" with the box "Validate user has access to the Taxonomy term" checked. Take the "Access Denied" action if validation fails.
4. As anonymous user, go to /validator-test/1 where you should see the article listed.
5. As admin, unpublish the tag you created in step 2.
6. As anonymous user, re-load /validator-test/1 which you should no longer have access to. But you do!
7. Clear cache and reload as /validator-test/1 anonymous user. Now you correctly lack access.

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

Hi, @Nelo_Drup! I don't plan on adding any options that affect anything other than the background region.

I think the best way to make this work like layout_builder_awesome_sections would be to create your own custom module that extends the AwesomeLayout layout class using the LayoutBgTrait as documented in one of the Layout BG example modules:

https://git.drupalcode.org/project/layout_bg/-/tree/8.x-1.x/examples/lay...

With any luck it's not any more complicated than that, though I can't make any promises. :)

The patch in #12 worked great for me!

My use case is that I have a link field on an "alert" content type that indicates which pages the alter should appear on. An alert with no link specified should appear on all pages.

I attempted to use views_contextual_filters_or → to OR a second contextual filter that returned a default value of NULL, but Views did not seem to want to allow a contextual filter to be NULL. This patch though did exactly what I needed.

I did the deprecation thing, but I'm not so sure about the tests anymore. I'm just not sure how this fits into the "@group legacy" system. It's not that we want to necessarily remove any of these test cases in 12.x; instead we want to update what we expect.

Hurray!

Here's the comment for the info alter, the example being identical to what is covered in the test.

/**
 * Create :foreach process plugin variants that do not handle multiples.
 *
 * For example, if the my_array is an array [4, 2]...
 *
 * @code
 * my_dest:
 *   plugin: evaluate_condition
 *   condition: greater_than(3)
 *   source: my_array
 * @endcode
 *
 * will result in my_dest being TRUE since any array is greater than 3.
 *
 * However...
 *
 * @code
 * my_dest:
 *   plugin: evaluate_condition:foreach
 *   condition: greater_than(3)
 *   source: my_array
 * @endcode
 *
 * will result in my_dest being [TRUE, FALSE] since 4 is greater than 3 while
 * 2 is less than three.
 *
 * Implements hook_migrate_process_info_alter().
 */

danflanagan8 → created an issue.

Thanks, @mikelutz. I really like the suggestion to require a string and add the deprecation. I suppose the deprecation needs a test.

If I can (continue to) be pedantic, though, the explode method indeed accepts non-strings as long as they can be cast to non-empty strings. That's why I liked the try/catch thing (i.e. Whatever Works). However, I like requiring a string as the delimiter a lot better than that (where "that" could refer to the try/catch or the linked movie).

fix extra nesting

danflanagan8 → created an issue.

This is a dupe of a recently fixed issue! 🐛 Headers set by Authentication plugin override custom HTTP headers. Closed: duplicate

Closing.

Per the plan #3263852: [policy] Conditions plugins in Migrate Plus → , we should support this as a new migrate_condition plugin in Migrate Conditions.

I would argue that this is a bug in the yaml system rather than migrate_plus in particular. I ran into this a few years ago and opened #3244757: Multiline YAML syntax is buggy with single newline character → against core.

@prabha1997, I'm clearly actively working on the issue, so it would be more productive for you to help move a different issue forward. I'll assign it to myself though so no one has to read between the lines. Further, you didn't even the change that @mikelutz suggested.

@mikelutz, I'm concerned that your suggested check prohibits the use of integers as the delimiter, which is neither prohibited by the explode method nor by the current explode plugin.

What would you say about this as the constructor?

/**
   * {@inheritdoc}
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition) {
    if (!isset($configuration['delimiter'])) {
      throw new MigrateException('delimiter is empty');
    }
    try {
      // Validate that the delimiter is legal by exploding a test string.
      explode($configuration['delimiter'], '');
    }
    catch (\Throwable $t) {
      throw new MigrateException('delimiter is invalid');
    }
    parent::__construct($configuration, $plugin_id, $plugin_definition);
  }

I opened up an MR with some tests and the fix from @DamienMcKenna in #2.

The test only job went as planned (https://git.drupalcode.org/issue/drupal-3412309/-/jobs/1828991) but I had a boneheaded cs violation for which I had to push up a new commit. So the normal pipeline is still running on that.

As kind of a self-review, this reminded me of and old issue about making the exceptions more consistent from process plugins. See #3247950: Throw consistent exceptions on invalid process plugin configurations → . With that mindset, I'd rather move the validation of the delimiter to the constructor. And once it's in the constructor, I'd rather just put a test call to the php explode function within a try/catch. Something like

try {
  explode($configuration['delimiter'], 'dummy text')
catch (Throwable $t) {
   // throw whatever new exception we want
}

Then we're truly honoring whatever explode allows as the delimiter. That feels a bit out of scope though. 'Course we could always change the scope if that's appealing! That would also protect against someone using an array as the delimiter, which probably just results in an uncaught exception currently.

The issue this was postponed on has been fixed. Un-postponing!

I see two comments form migrate system maintainers indicating that documenting the distinction may not be worth it. I think that's especially true given the details of how the related issue was fixed.

Here's a process-plugin approach to the same basic problem: ✨ Add set_on_condition process plugin Needs review

I'm not sure it's a good idea, but I wanted to throw it out there.

This reminds me of an older issue I looked into at one point, 🐛 Error if migrate destination id contains NULL values Active .

Is this a dupe?

My comment at #39 → on that issue has some especially relevant details.

The patch in #35 did not work for me. I get the Views showing up in the linkit widget, yes, but I can't select anything (View or otherwise). I keep getting js errors like:

ReferenceError: substitution_extra is not defined

which points to the new line in js that the patch adds:

        $getAttributesInput('data-entity-substitution-extra', $context).val(substitution_extra);

and then sometimes I'm getting this js error:

Pattern attribute value \<front\>|\/|\/[^\/]+.*|\?.*|#.*|.*\(\d+\) is not a valid regular expression: Uncaught SyntaxError: Invalid regular expression: /\<front\>|\/|\/[^\/]+.*|\?.*|#.*|.*\(\d+\)/v: Invalid escape

Should be in beta9.

I got permission from the author to add the file here. It's a zip since I'm not allowed to upload the xml directly.

Thanks for the work on this one, @sthomen!

Darn. We just ran into a problem with the patch in #16. There was an error that was triggered while in the context of the huge uid and that resulted in an error when logging to the db:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'uid' at row 1: INSERT INTO "watchdog" ("uid", "type", "message", "variables", "severity", "link", "location", "referer", "hostname", "timestamp") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9); Array
(
[:db_insert_placeholder_0] => 9223372036854775807
[:db_insert_placeholder_1] => php
[:db_insert_placeholder_2] => %type: @message in %function (line %line of %file).
[:db_insert_placeholder_3] => a:6:{s:5:"%type";s:45:"Drupal\Core\Database\DatabaseExceptionWrapper";s:8:"@message";s:15217:"SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'uid' at row 1: INSERT INTO "watchdog" 
...

So PHP_MAX_INT may be too big. We may need to pick a uid that would be legal in the watchdog table. It looks like that's an unsigned int, but I don't know if that has different meaning for different database backends.

danflanagan8 → created an issue.

Hi @sthomen

The optgroup appears to be by design in ViewfieldWidgetSelect::ajaxGetDisplayOptions

I noticed and ignored that console log too. I'm still seeing it when I install beta7 though. I wonder if It's something I introduced while trying to do D10 compatibility. See #21 and #22 on 📌 Automated Drupal 10 compatibility fixes Fixed

I'll open up a new issue that handle that fix.

If you (or someone else) gives the MR an RTBC I'll put out a new release.

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

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

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

@sthomen, thanks for adding those thoughts!

The MR I opened has the essential function (though it's not obvious!) of assuring that `target_bundles` on the handler settings gets set to NULL rather than an empty array. NULL means that anything is allowed whereas an empty array means that nothing is allowed (even though views don't have bundles!).

I suspect that having `handler_settings` itself explicitly set to an empty array as you saw in your testing has the same effect. There's probably two arrays getting +'d somewhere.

If you ave time to test out the MR, I'd really value your review. Cheers!

A more successful fix is being attempted in the related issue: 🐛 Cannot select display of view in default value - possible regression Active

Note that I'm seeing a core bug surface: 🐛 Querying with NULL values results in warning mb_strtolower(): Passing null to parameter is deprecated Needs work

So if you get Deprecated function: mb_strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Core\Config\Entity\Query\Condition->compile() (line 39 of /var/www/html/core/lib/Drupal/Core/Config/Entity/Query/Condition.php) when testing this out.

I just opened an MR that I think addresses the bug.

I'm honestly pretty new to this module and haven't used it a ton, but it seems like we are doing things in field settings that might be more appropriate as handler settings for a custom entity reference selection handler. The root of this bug seems ot be that the default selection handler is being run even though it hasn't been configured properly and is (completely?) useless for us. The field widget is doing a lot of things that the selection plugin might be more appropriate for.

For now though just updating the settings form as in the MR should be completely undisruptive and seems to fix things.

@DD 85, it's because I turned the patch into an MR, made an update such that the tests pass, got an RTBC from someone, and then merged the MR.

@DD 85, please see the test results reported in #19. The patch in #2 is not sufficient.

Hey, @drunken_monkey. The patch in #16 fixed the bugs for me when using search_api 8.x-1.34. That release has the fix from the related issue.

I had a very similar experience to @sunlix. I had a menu block with a visibility condition restricting it to authenticated users. From time to time (I couldn't figure out a real pattern!) the menu would not be visible when loading a search_api View page as an authenticated user. Concurrently, a couple places that were leveraging the `logged_in` twig variable rendered as if the user was logged out. After applying the patch, the intermittent bugginess was resolved. Though, being intermittent, it's hard to say with 100% confidence...

I wonder what kind of test coverage we could cook up for this. I couldn't figure out a consistent set of steps to coax the bug out.

@Defcon0, I see the change to ViewfieldItem::defaultFieldSettings in beta8. It's more likely that the fix just doesn't work.

See 🐛 Cannot select display of view in default value - possible regression Active

Hi @maxilein. Thanks for the report. I reproduced easily. Not able to set a default value. Lots of console logs and db logs and a WSOD on save. Ouch! Clearly there's extreme bugginess with setting a default value.

I have to say I'm very confused as to how the patch https://www.drupal.org/files/issues/2023-08-21/3381787.patch → would fix this. That patch sets a handler on the storage, but the handler is related to the field config, not the field storage. So I'm confused.

I think Major is the right priority for this. I don't know that it's a true regression since beta7 had similar (if no identical) problems setting a default value.

Adding the related issue I noted in #35. The steps to reproduce on that one don't involve jsonapi which may simplify things.

This looks very much related to 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work . Actually they looks like dupes. Should probably close that one since it's way more recent. There's a patch on that one that might be worth reviewing though.

I found some nice related issues while reporting that one. In particular, I felt like the real solution was to be found in 📌 Introduce entity permission providers Needs work .

I ran into this issue. I have a view block with an exposed firm and views_filters_summary header. When I click the X to remove one of the filters, I end up getting directed to the search page! That's because there's an exposed form in the page header for the search page. That's pretty typical.

Agreed that the selector needs to be a bit smarter for the form's submit button.

I changed the word "filter" to "form" in the title and the IS because that's more accurate. And note that one does not even need to have multiple instances of the views_filters_summary area to get this bug. Like I reported, it's happening with the search bar in my site's header.

The patch in #22 saved the day for me! It would be great to get this in a new release. ;)

I was running into what seemed like this exact bug with 8.x-1.7 and bef 6.x,. I had to uncheck a checkbox twice in a row if I wanted the View results to update.

But then I discovered it was really a different issue: 📌 AJAX GET in core: Alter ajax url query parameters Needs review

Posting here in case it helps anyone else!

The patch in #28 unfortunately did not work for me. I'm using facets 3.x (i.e. as views exposed filters) and views_ajax_history.

When I try to un-check a facet, the view re-loads with no changes. If I un-check the facet a second time, then the view re-loads correctly. This happens with or without this patch. Not sure if it's a facets bug or a views_ajax_history bug.

I applied both patches. I keep seeing this warning:

Warning: foreach() argument must be of type array|object, string given in Drupal\facets_exposed_filters\Plugin\views\filter\FacetsFilter->acceptExposedInput() (line 141 of modules/contrib/facets/modules/facets_exposed_filters/src/Plugin/views/filter/FacetsFilter.php).

The text of the pills looks good, but the X links don't seem to be working for me. I'm new to views_filters_summary so I don't know if that is necessarily related to facets.

Hi @xamount. I'm sorry to say I don't have a great answer. I'm doing my best to maintain the module, but I can't say I'm a real expert in this area.

I know you upload a file through the UI and the file is stored in the private file directory and the path to the file is stored in state.

If that file with the api key makes it to your dev environment, I suppose it's possible it would try to send something to Google?

You could always try config_split or something similar to prevent the module from being enabled on non-prod sites.

I don't know anything about how subdomains would be handled.

danflanagan8 → created an issue.

danflanagan8 → created an issue.

Hurray! I wasn't sure this was going to make the tests green, but it did! Feeling much better about D11 readiness now that tests are all passing on D10 again.

danflanagan8 → created an issue.

danflanagan8 → created an issue.

danflanagan8 → created an issue.

Leaving this open in case anything new comes through.

> But looking at the solution this would only work for nodes right? What about other entity types?

The feature itself only exists for nodes, and the option text is clear about that:

Load default filter from node page, that's good for related taxonomy blocks

So that's all the caching we have to worry about too.

I just tagged 8.x-3.0-beta8 → , which includes this fix. Thanks, all! 🇺🇦

I just tagged 8.x-3.0-beta8 → , which includes this fix. Thanks, all!

Tests are green again.

I turned the patch into an MR, but the tests are failing because of schema issues:

> Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.storage.node.field_view_test with the following errors: field.storage.node.field_view_test:settings.handler missing schema

I'd like to fix that before merging.

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

danflanagan8 → created an issue.

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

Add parens docs.

danflanagan8 → changed the visibility of the branch 3427374-argument-default-cache-tags to hidden.

I put in an MR with a fix and some tests.

I've thought a bit about my claim in #6 that we don't need the cache tags of any of the terms. We don't access any fields on any of the terms that might be returned, so the only relevant aspect of the terms that might change is the publish status. Of course, publish status would be handled by the validator plugin, not the argument_default plugin. All in all, I'm pretty confident we don't want to use the terms' cache tags.

danflanagan8 → created an issue.

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

danflanagan8 → created an issue.

I have something working for this. It's another fieldset in the "Populate from real migration" drawer. The only "constraints" you can add are equality, so it's not too sophisticated. I don't have time to add tests atm, but manual testing has been good.

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

This is like the 10th time I've done this today, so I was able to knock this out real quick.

I set up a monthly job that should test against the previous, current, and next minor versions as well as the next major.

danflanagan8 → created an issue.

danflanagan8 → created an issue.