🇳🇱Netherlands @Lendude

So now the filtering happens in two spots right? It still happens in Drupal\views\Views::getEnabledDisplayExtenders()

I would think we would want an update hook here that cleans the existing config and then we can remove the filtering in Drupal\views\Views::getEnabledDisplayExtenders() right? Scope wise it makes more sense to me to do that here than in 📌 Add validation constraints to views.settings Postponed but ¯\_(ツ)_/¯

🌱 [meta] Add constraints to all config entity types Active only talks about the Views entity config, but ideally we should also implement constraints for all Views plugins, so repurposing this for that

Since most of Views is config, 🌱 [meta] Add constraints to all config entity types Active should, one day, provide this I hope

@Matt B thanks for reporting this regression. We can't revert this anymore and reopening a long running issue like this will lead to mistakes. I've opened 🐛 [regression] "Content: Has taxonomy term ID (with depth)": instead of displaying the untranslated term title, no term title is displayed Active to address this issue, can you provide a clear description of what broke and some steps to reproduce your issue there? Thanks!

Lendude → created an issue.

Reopened the referenced issue, lets see what others think about keeping that open or if others think it can indeed be closed

Didn't realise this got closed, I don't think it should be.

#3065720: When creating a Page View in the wizard and setting a path with leading slash (/) the created View display ends up with a double-slash (//) → was about things going wrong if you added a leading slash, so we strip any leading slash now. But the problem here is that the rest of Drupal actually wants you to add a leading slash when you set a path (menu items, aliases), so the UX in Views is reversed from the rest of core. So this issue is about getting Views in line with other parts of core. And I don't think that has been addressed, so it shouldn't be closed.

The questions in the IS remain, are we ok with Views doing it in another way than the rest of core or do we want to get it in line.

Hmmmm I'm not convinced the referenced issue could be closed....I'll comment there

Nice, the test coverage isn't triggering the bug it seems though, the test-only run https://git.drupalcode.org/issue/drupal-3319601/-/jobs/2003864 is green

@nlisgo ha! good point, I miss read/remembered what needed to be done here.

So the conclusion in #6 was that role='alert' is fine, I thought the next step was to add that, but we already add that, so we don't need to do anything here, I think. So closing this. Please feel free to reopen this is you think there is still work that needs to be done here

Blocked by: Currently no way to update Server Side Messages through the JavaScript Message API

Figure out if this is within the scope of this issue (ie. there will be no generic way to do this, IFE just needs to figure this out for themselves) or if a generic issue should be created for this that IFE can then use.

I think the remaining failures mean the database fixtures need to be updated? I think?

As the test failures indicate, we missed removing some, or some new instances of Views with default_argument_skip_url got added after wards

Something like this I guess

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

Thanks for the feedback, pretty clear on the next steps! Moving this to needs work.

Nice, good to see coverage added.

The Counter plugin uses UncacheableFieldHandlerTrait but this seems to break that, since the existing tests now require cache rebuilds. That feels like a regression we need to look at.

Jean-Paul Vosmeer → credited Lendude → .

So this block now no longer exists in the UI? Is that right? So do we postpone this on 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Postponed ?

Nice to see this moving!

would the current approach actually result in duplicates and would that cause problems/disruptions if it did?

Yes and yes. Even with just core, this happens. Core only has 2 computed base fields currently, URL alias and Content Moderation state. To test, just use an Umami install with the current MR and you will see these fields get added for nodes. It will also show the problem.
The URL alias field will be there, but won't have any allowed formatters, so you can't make it output anything, which is very confusing.
The Content moderation field will be there twice and you have no idea why and what the difference between the two is.

So it won't break anything, but UX will take a hit as it stands.

I also thought about how we can detect potential problems and for the lack of formatters that might be doable, but the duplicates, I don't see how we could do that reliably. If people are altering things, we have no easy way to detect that, and no way to determine if the change was intended even if we do detect a change.

So some things we could try:
* Check for available formatters before adding the fields (and not add it if there aren't any)
* Expand the description or field name when we add the Views data to indicate that this a computed base field, so there is at least some indication in the UI of where this is coming from

Tried this on MySQL with the node table, and querying the 'title' and 'type' fields, and that works as expected.

So there is something else going on than just loadByProperties not being able to handle two strings.

Is the custom entity doing something special? Does it have it's own entity storage that does something? Is it only a problem on postgres?

I looked at the code and all it does is cast values to arrays, so that should matter.

+1

Should be easy enough to add another guard statement to views_field_default_views_data I guess

Can we get a real issue summary here? When is this happening? What other modules are you using? Can you reproduce this on a clean Drupal install?

Just adding an isset() is usually just masking the real issue.

larowlan → credited Lendude → .

If you have a node_access implementation on your site, couldn't you just remove the status filter from the View and the correct nodes would be in your View? ie. The nodes that you have access to, independent of the status? Hmm probably only if you do something with (un)published in node_access I guess

Also, this list: '***UNPUBLISHED_NIDS_ACCESS_GRANTED_BY_NODE_ACCESS***' can potentially get HUGE, at how many unpublished nodes will this fail? Depends on max_allowed_packet I guess, but it will fail at some point....

Also, with the logic that was added in #3030477: Views filter "Published status or admin user" not checking "View any unpublished content" permission → and if we now add this, the responsibilities of this filter just seem to be getting too much, they certainly aren't just what the description of that filter says.

Sorry, nothing concrete to add, just general worries. Also, I see the problem, we've run into it in our projects too, just don't see a clean solution at the moment

Did you run updates? That setting was removed in 📌 Get rid of using 'views.settings':skip_cache in ViewsData Fixed

Agree with @acbramley, the Media Library sounds like the wrong widget/field type here, it should just use a file field. Media is about reusing.

And the described results seem inline with what the permission set dictates. Sure, they give confusing results for the end user, but I don't see how we would change the outcome short of trying to guess the intent based on a certain combination of permissions chosen. So "works as designed" I'd say.

Since it seems to break on \Drupal\views\Plugin\views\query\Sql::create() maybe a patch that was updated (but might be broken) during the upgrade?

Does the problem go away if you remove all patches from the project?

Didn't try, but even if we would fix the access check we would then probably run into caching issues, since the validation cache contexts wouldn't bubble currently due to #3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page →

This behaviour is not specific to trimming, rewriting the result to a link acts the same way, it works on individual results, not the end result.

The way to work around this is by hiding the multi value field and then using it as a token in a second field (based in an ID field for example) and then trimming the value of the field where you are using the token.

I am not in favour of adding more settings to the multiple value form. If we want to fix this in a way that doesn't require a workaround I'd be looking more to a single checkbox that does something like 'Apply all Rewrites to the concatenated values' and then we don't need to change this one functionality at a time.
Not saying that would be easy, because I don't think it is, but that would not make the Views UI any more crowded than it needs to be.
My 2cts.

Steps to reproduce:
* Create a View of nodes
* Show fields
* Show the nid and title field
* Add a contextual filter 'Content: Has taxonomy term ID' and set default value to ' Display all results for the specified field'
* For the title field set rewrite results: Output this field as a custom link to /node/{{ nid }}?term_id={{ arguments.tid }}
* Create a page display
* Go to the page and see that 'term_id' is added to the link URL on the title, bt with no value

Didn't try the patch yet

As pointed out in #3, 'Content ID from URL' won't work with products, because it will look for a node ID, not a product ID (naming things is hard)

The hidden selector 'Profile ID' is not something that comes from core, no idea what puts it in your form, but it is probably something from contrib or custom code. So best start looking there.

larowlan → credited Lendude → .

Screenshot of the failed test page: https://git.drupalcode.org/issue/drupal-3439427/-/jobs/1271159/artifacts...

PoC to show we can do this, needs clean up

Lendude → created an issue.

Hide patches

Colleague just ran into this, lets fix it. Moved #30 to a MR

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

Since we no longer need the needs review status to trigger test running I'm resurrecting this.

The latest patch and the MR patches posted in #113 still break the media library search

Sounds good. The code weight is indeed low, it has some test coverage and the number of open issues seems manageable. Plus, it's a very useful feature to have!

On a View with about 40 displays but not that many fields I have 8791 input vars, so hitting 10000 doesn't seem impossible, for me it also needed more than 256M of memory to actually load the form in the first place....

If you add something like

    if (!empty($input)) {
      var_dump(count($input, COUNT_RECURSIVE));
      die();
    }

to \Drupal\Core\Form\FormBuilder::buildForm after $input has been build and then submit the form you can get an indication if this is the problem.

Either way, we need better steps to reproduce this, because for most Views it works just fine. So for this to be an issue other than #1565704: Core interfaces can go over max_input_vars → , please provide steps to reproduce this with a View that has less than the max_input_vars but still won't save and doesn't give an error message.

Applied the patch in #37, unfortunately still runs out of memory. No time to test further I'm afraid.

Updated the IS, code looks good.

We were using 📌 Use 'From' email address for the sendmail POST user ID RTBC before, hence the working setup before updating, but that got reverted, so this is a new attempt at that but using the new helper method

So redid that patch to use the new helper method. Also redid the current logic for the default from address since the ?? doesn't do what the code hopes it does since default_mail will get set to an empty string.

Added logic for handling a given 'from' address

NB. this seemed to work previously, but looking at the old code, it was never supported, so I might be missing something

Lendude → created an issue.

We updated to 10.2.12 and one map displaying ~600 pins stopped loading, giving out-of-memory errors where previously it was loading fine. Tracked it down to this change. Something here makes the caching go crazy and loopy until \Drupal\Core\Cache\CacheTagsChecksumTrait::calculateChecksum runs out of memory. Not sure if this is the fault of core caching, search API caching or how this patch changed things but here is a patch reverting this change for people running into similar problems and don't need this functionality

Yeah that is more readable, changed it, and added a small comment

I see plenty of coverage for this method in \Drupal\Tests\path_alias\Unit\AliasManagerTest but none that tests the exception being thrown (as far as I can tell), so probably good to add a small test method there that tests this

All other 'delete' operations in core trigger a dialog as far as a little clicking around could show me, so cancel there just leaves you on the current page. This is also what the delete option in the Views list does. Wouldn't it make more sense/be more consistent to have the delete option in the View Edit screen also trigger a dialog?

Managed to create MR now, updated the IS a bit

This would show something like this in the UI:

I'm not sure if using $plugin->pluginTitle() in the selector is great, because there are no further explanations about what the options in the list are, it's probably best to be verbose there.

I would like to see it used in the admin summary, so that you can see in the Views UI which (if any) default plugin has been selected without having to open the edit dialog.

A use case I can see: If you have multiple fields in a row and only want to show the wrapper for that field if it has a value?

Since it would be up to the plugin rendering the row to do something with the information from the fields, it would be very hard to bubble this setting up to the row consistently.
That would need to be implemented in all Style and/or Row plugins individually, sort of like what the Table Style does with the empty_column setting.

So to me this is 'Works as designed'. The setting in the field should control that which it has control over: only that field.

But yeah, better wording might help, although the description is already pretty extensive: "Enable to hide this field if it is empty. Note that the field label or rewritten output may still be displayed. To hide labels, check the style or row style settings for empty fields. To hide rewritten content, check the "Hide rewriting if empty" checkbox."

larowlan → credited Lendude → .

Not 100% clear what you would want/expect to see but it sort of sounds like you want the exposed filter to do a POST request and not a GET request.

You can probably do that through a form alter hook, not sure how well it will play along with other parts of the View if you do that.

Tried to reproduce, and this seems to be working as designed. The option that is enabled in the screenshot and the steps is about hiding an empty field, which it is (the row is empty in the screenshot). What is not hidden is the empty row. There is nothing saying that this setting should bubble up and hide the whole row (if all fields are empty and all fields have this option enabled?). Right?

If you want to hide rows that don't have content in a certain field, that would need a filter.

Am I reading this correctly or is there something here that I'm overlooking?

I agree with #2, feel free to reopen this is if you feel this is a different issue 💬 Views UI - Parameter "view" for route "entity.view.edit_form" must match "[^/]++ Closed: works as designed

It's not available when the currently active display is an Entity Reference display, I do see the option if I use it on a Page display. Right? Or am I misreading?

Test-only job fails as expected https://git.drupalcode.org/issue/drupal-3420230/-/jobs/915521

Added some test coverage and simplified the merge statement

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

Sounds a little edge case to me, don't think it makes the 80% criterium, especially with token support thrown in. Might be more user friendly with a select element showing available languages

It would need some things I think:

• should only be shown on multi-lingual sites
• yeah validating if we have valid input sounds good, just hoping it falls back sounds bad
• tests
• a schema update
• an upgrade path for the schema change
• a test for the upgrade path

Lendude → created an issue.

larowlan → credited Lendude → .

Please search the Views and Views UI modules (and maybe others?) for 'Link display' and update all occurrences everywhere. Please make sure the changed sentences still make sense, not sure a find/replace is going to work everywhere

Did anybody ever look into moving this out of the pager section? I agree with @dawehner in #1 that this feel like a weird place for this.

Do we need to think about updating the machine name of the setting too? We now have a big difference between the name of the field in code and the name in the UI which can lead to more confusion but now from a DX perspective

Do we need to run the new label past some UI experts and make sure we all agree this is the best we can come up with, 'Destination' feels a bit vague to me.

📌 Increase or remove default textfield #maxlength=128 Needs review might solve this too

Feels similar to 🐛 Views RSS Feed Fields adds tag. Active and probably also caused by 🐛 Allow TimestampFormatter to show as a fully cacheable time difference with JS Fixed

Thanks for opening this, it reads like a duplicate of 🐛 Ellipsis in pager template fails accessibility tests Needs work so closing for new, feel free to reopen this if you feel this is addressing a different issue.

Well if there is a use for it, we can always undeprecate it (or in this case, never deprecate it in the first place), or if it needs a string version of this we should add a string version and put it in the pgsql module where this seems to belong (is PostgreSQL the exception here or is MySQL? How does SQLite handle this?)

Also, I think the general consensus is that int joins are more performant than string joins, so what performance impact does casting everything to a string have? We should check.

How is this handled outside Views? It's not like Views is the only place you can make joins. Is there maybe some method we could/should add to the database drivers that handles this?

I would like to see test coverage added that actually shows we need this, so something that would fail without this fix. All the modified test coverage changes only show that the ':text' is added and this doesn't break existing queries.

Apparently doing a merge to resolve conflicts makes spellcheck croak.... 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active , so needs a rebase

Did some clean up, no idea what spellcheck is complaining about.

I think we still need an upgrade path test for this

Rereading this, feels like a duplicate of 🐛 Switching on aggregation generates fatal "Column not found: 1054 Unknown column" SQL error when using multi-column Fields Needs work can somebody check if the work there helps fix their issues?

Nice find, fix looks good, but we should probably add a test for this, shouldn't be too hard to add something to \Drupal\Tests\views\FunctionalJavascript\ClickSortingAJAXTest I think?

Tried to reproduce:
* Clean Umami install
* Enabled Comment module
* Checked key_value table, comment entry was there
* Uninstall comment module
* Checked key_value table, comment entry was gone

Can you reproduce this on a clean Drupal install?

Added some more modules

This test printed output: int(37)

Checked all modules that implement hook_views_data, think this get all of the ones that don't need extra handling or just reuse existing plugins...I think

Forgot to untag

The "Views Default Argument: Entity Field Values" module already provides this functionality and much much more, see https://www.drupal.org/project/views_arg_entity_field →

I don't think we should be adding something so Node specific to core. The problem with these default arguments is that they are not filtered and all of them always show up on all fields even when totally inappropriate (filtering a date field by current node type when viewing a taxonomy term anyone?), so I'd prefer the list we have by default in core to stay as small as possible. Yes default arguments are great tools but having a generic solution like the module provides is, I think, a better way forward than adding individual fields to core.

This checkbox comes from \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceLabelFormatter, Views just reuses the field formatter, so not sure if this is something Views should be handling or the FieldFormatter itself.

@larowlan is 📌 Fall back to 'edit-form' as default $rel in EntityBase::toUrl() Fixed the change you are referring to? The formatter seems to catch the new exception thrown there so not sure.

@daffie the visual representation leaves a little to be desired to make it clear, but if you look at the top level Postgres build you can see that the normal test runs pass and only the test-only run fails, as we would hope

https://git.drupalcode.org/issue/drupal-3415118/-/pipelines/79102

Hiding patches

PostgreSQL test-only fails as expected: https://git.drupalcode.org/issue/drupal-3415118/-/jobs/653536

So this looks good to me

Nice, moved to a MR and lets see if the test run better there, works fine locally

Nice find, we need a test for this, I've already written one in 🐛 A number of Views combined fields filter operators are currently pointless to an end user Needs work that just hasn't landed yet.

Probably easiest to grab the REGEX test from that and add it here and see if it indeed fails on PostGreSQL

Can we use \Drupal\views\Plugin\views\join\CastedIntFieldJoin for this and avoid adding pgsql specific code to base plugins?

Moved code to a MR to provide some feedback

This seems to have been fixed in #2899248: Don't reimplement validation rules for workflow state add/edit forms in ::copyFormValuesToEntity → where this was added

if (!$form_state->isValidationComplete()) {
      // Only do something once form validation is complete.
      return;
}

Am I reading that right or is there still something broken here?

#3 and #9 outline nicely why we can't just use the field's labels, because the same field can have multiple sets of labels. So the goal set out in the title I don't think we can do.
The option set out in #9 where we provide fields in the boolean filter where you can configure this could work, but does that really offer much more value over just using a grouped filter for this? I don't think it does, so "Won't fix" for me, unless we can think of way to determine the values we should be using here

Did a check:
Investigation: See for example https://www.drupal.org/project/drupal/issues/2404603#comment-9576291 → for the test in an earlier form, where the two parameters made sense (they contained different things)
Conclusion: leftover code from previous iterations of the test that never got committed in that form and was missed in review

Added to the IS

larowlan → credited Lendude → .

Custom Date field

What does that mean? Is it a date field added through the UI or a base field or some type of custom date field you've build?

Per #4, please add some steps to reproduce this on a clean Drupal install, that would really help

Check all your Views that have a taxonomy term filter and try to resave them manually. Does that work? Are there any broken handlers on them? After resaving manually, does the update run?
All the update does if resave the View if it has such a taxonomy term filter, and apparently that is failing, usually this is due to a pre-existing problem in the View and has very little to do with the actual update. The update just surfaces the issue.

Can you provide steps to reproduce this with just Drupal core? The stack trace is using paragraphs

We actually have a Views test that uses a batching action: \Drupal\Tests\views\Functional\UserBatchActionTest::testUserAction

Not sure if this issue can be replicated with just core, but if it can, it would be good to add a test at some point.
I agree that we don't need to add coverage in this issue, fixing the regression is more important, but I would suggest making a follow up issue to expand the coverage in that test a bit, it's very minimal as it stands.

We really need some steps to reproduce this if we are going to keep this open. Is this still relevant? If so, when?

Can we track down what changed here in 10.2? Neither files touched by the current fix were changed recently, it would be nice to track down what caused this so we can maybe prevent other side effect and make sure we are on the right track with fixing this here and not just band-aiding a much bigger issue