🇳🇱Netherlands @Lendude

So, I'd say RTBC

I feel this setting is related to arguments being able to provide their own defaults, but see 🐛 UI fatal caused by views argument handlers no longer can provide their own default argument handling Needs work , that has been broken/removed since before 8.0.0. But as that issue shows, this argument plugin has other dead code in it, and I think this property is part of that.
I feel it's safe to remove this.

This has been replaced with a log, which feels fine, changing it to an exception would probably run the risk of breaking things so not sure that would be worth it

Yeah that clears up my question! Thanks.

This should fix the mismatches at least. Setting the default to either 'or' or 'and' feels like we would be assuming a little more than we are doing now, so an empty string feels safe.

lendude → created an issue.

Yeah deprecating this makes sense. And that seems like a good spot to do it.

That said, how do we want to proceed when removing this? If this is actually used in the wild (however unlikely), when we remove this functionality, it will go from an access callback to returning TRUE. Since we are dealing with access here, this could expose something somebody wanted hidden. Again, very unlikely, but something to consider, maybe ask somebody from the security team if we would be comfortable with this.

Not 100% sold on the manual list of reserved paths, now when we remove 'profiles' from core for example, we need to remember to update this list (we won't). Is there no way to get this list dynamically?

Ok nothing breaks, this still needs test coverage though.

Pushed first attempt at a fix, lets see what breaks.

No new test coverage yet

lendude → created an issue.

smustgrave → credited lendude → .

The overridden sections are in italics currently. So isn't that our indication?

At step "4. Now go back and try to change the aggregation type to "Sum", "Average", or any other." the dialog doesn't close and if I look in the watchdog I see:

TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in array_filter() (line 728 of /var/www/html/web/core/modules/views/src/Plugin/views/field/EntityField.php).

So definitely something broken here it seems like.

Removed the issue that was marked as related since it has nothing to do with this (totally different kind of aggregation)

Tried to reproduce this on D11 install

Steps done:
* Switched the content view to use Ajax
* Exposed all pager settings
* Went to admin/content
* Changed some pager settings
* Used the pager
* View still uses Ajax

Am I missing a step? Can you reproduce this on a clean Drupal install?

I agree with @xjm this isn't really a bug, so moved to task, might be a feature ¯\_(ツ)_/¯

@dawehner seemed to be in favour of this back in the day, and so am I, so removing the subsystem tag. I understand the initial thought behind not showing this, but I believe the Views UI is still plenty complicated without showing this column. And as people have pointed out in this issue this can lead to information being hidden. Also, it always felt a little arbitrary what was considered 'Advanced', since, lets be honest, the whole Views UI is pretty advanced :)

@yoroy has commented here so not sure if a usability review is needed, but happy to follow @xjm on this, so tagging for usability review

Can't think of (or find) anything either that could possibly rely on this. So great clean up as far as I can tell.

Left a small comment on de MR. Might just be me reading the flow wrong.

Yeah the code in #3 pretty magic, but it's not too bad I feel. It does need some figuring out what's going on, so if others agree this is the best way to go, it would benefit from a comment or being moved to a private method to document what is going on I think

Applied the patch, and now I have working field groups in my media library! Sweet!

Don't know if it is too generic now, but it works and clicking around the site a bit, I don't see it breaking anything.

If I understand you correctly, what you expect to see is not how it's supposed to work. The operator is only for the resulting query, not for which options are shown in the filter, so if you limit the options it will always show the options you checked, not the reverse of what you checked.

Am I understanding your issue correctly?

Steps to reproduce don't show any errors for me. Anything else that is needed to break this?

We shouldn't change the behaviour for the mini pager here, it is out of scope. The mini pager doesn't do a count query and it should stay that way. And if we would want to change that, it should be its own issue.

This needs tests, and upgrade path for the new setting and a test for the upgrade.

Thanks everybody for working on this.

If I'm reading the diff correctly (and let me know if I didn't!), we go from a situation where we can show different translations than the language of the 'parent' item to where it is only possible to show the related translation with a language equal to the 'parent' item. So if you want to show all translation related to an item, now you can't do that anymore. Right? That sounds like a loss of function, and we should determine if we are ok with that
Also, I think but didn't test, if there is no translation in the related item that matches the language of the 'parent', you will now get no results, that sounds like a regression we should address.

I feel that the underlying problem is that entity references are not made to a specific translation, and I don't think it is up to Views to decide/hardcode what the user meant to refer to.

Core cspell ignores binary stuff it looks like (see .cspell.json in core):

"ignoreRegExpList": [
      "^msgstr .*",
      "!!binary .*",
      "%[0-9][0-9A-F]",
      "\\Wi18n",
      "\\x{[0-9A-F]{4,5}}"
    ],

Wouldn't that work?

I think changing this to a normal sting is hard, see the comment in views.field.schema.yml, we need to allow the control character:

    format_plural_string:
      type: plural_label
      label: 'Plural variants'
      constraints:
        Regex:
          # Normally, labels cannot contain invisible control characters. In this particular
          # case, an invisible character (ASCII 3, 0x03) is used to encode translation
          # information, so carve out an exception for that only.
          # @see \Drupal\views\Plugin\views\field\NumericField
          pattern: '/([^\PC\x03])/u'
          match: false
          message: 'Labels are not allowed to span multiple lines or contain control characters.'

I just checked and if you are moving things one by one via /admin/config/development/configuration/single/export, you are correct, I don't see an export feature for translations there. But that goes for all entities, that has nothing to do with Views specifically.

And the workaround would be to export the full config and manually grab the translation files from there.

The examples @smustgraves provides are for interface translation and not config translation, so I don't think those would apply here, but if they are I would recommend using https://www.drupal.org/project/potx → just to make the list complete

Sorry, I still don't understand the problem. Views translations are like any other config translation, and they get exported along with all the other config when you export it. This works fine.

Are you exporting config one thing at a time or something like that when you move things from dev to prod? And not the whole config folder at once? I've never done that (and that sounds like a lot of work), but in that case you might have to export the translations separately since they are their own config object I think.

smustgrave → credited lendude → .

Setting to active since there is no MR yet

This is making my duplicate-sense tingle, but can't find anything. Still, worth digging in old issues, might help find something I think.

I would be against doing this in Drupal core, it adds more options to the Views UI which is already too full of options, and this feel very much like an edge case. If you need this in a project you can just add a custom area plugin that does this and use that, right? Just my feeling.

Back to needs work for the tests.

Left a small comment, change makes sense.

We still need to verify that this is a core bug, we need steps to reproduce and to make sure this statement in the IS is actually true:

and it is possible that $data->{$this->base_alias} can be NULL

Why is that NULL? Is the bug that its NULL to begin with or that we are not checking for NULL?

smustgrave → credited lendude → .

Try to resave your Views all manually and see of any of them break, that might find you the broken View

The steps read like this would break for all Views using pagination and Ajax, which is obviously not the case. So we need steps to reproduce this on a clean Drupal install, because just adding pagination and Ajax won't break the pagination (we have coverage for that).

This would probably depend on 🐛 CKEditor not available for Views "text" areas Needs work landing.

Yeah I agree that this should have stayed in the Views queue. I does however depend on the dialog-in-dialog being fixed

This will need some test coverage and looking into the test failure.

With the coming of Drupal CMS we should just nuke this View I recon. Along with some other Views that ship with core.

This @todo no longer exists and there is a lot of logic handling Views, I'm guessing #2341323: Adapt the references field / table names in views, when corresponding entity schema changes → took care of this so we can close this

🐛 Views exposed text filter set to required shows an empty error and form error on page load Needs work maybe not exactly the same issue but certainly related (and might fix this too)

Moving to PMNMI to see if this occurs in a scenario that is supported.

Adding some related stuff, the OG issue 🐛 When deleting a content type field, users do not realize the related View also is deleted Fixed and it's spin offs #2575547: Formalize patterns for delete/uninstall of configuration dependencies → and 📌 Add a 'disabled' section to config changes page when removing config Needs work

Pushed a test using a container and added support for this in FormHelper, not sure a growing list of exceptions to the rule is the best way to handle this, but ¯\_(ツ)_/¯

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

smustgrave → credited lendude → .

Well, it's green but had to change the actual check if we want to add the class, didn't check if this had other effects

Not confident enough in my css skills to mark RTBC, but checked, and it fixes the issue in the Views wizard at least

This seems to have broken/stated showing some buttons that need to be hidden in the Views wizard:

Should open a follow up or is this a revert and fix?

Trying to update all the existing config which we also missed and the new deprecation now nicely highlights

We looked at this together at Drupal Dev Days together, and it looks good now, good to see that existing test still pass

Opened 🐛 Add ViewsConfigUpdater deprecation support for update_table_css_class Active to address this lack of deprecation, also it's not being run from updateAll, so new module config is not getting updated either it looks like

lendude → created an issue.

Ok that sounds good then :)

The fails in Drupal\Tests\workspaces\Functional\WorkspaceViewsBulkForm feel like related failures, so moving back to needs work to check those

Since something somewhere would have added that destination to the URL, should we expect this to be passed on the the redirect? Might be out of scope a bit, since ignoring it is certainly better than what happens now.

Added some other issues that report the same error, but none give any steps to reproduce this (one says a cache clear fixes it), so it's still unclear what scenario's trigger this, and we really need some steps to reproduce before we can tackle this

lendude → created an issue.

#12 reads like the solution shouldn't be what I see in the commit in #10, we shouldn't just revert to an empty array to ignore the problem, we should probably fix the submit flow described in #12 right? EntityField::buildGroupByForm() should be called or EntityField::submitGroupByForm() shouldn't not be called? Am I reading that right?

smustgrave → credited lendude → .

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

How about other styles that also use fields but don't have a options settings form that would allow this? Would you still need to do that using the old method? That might get confusing.

Also, I feel, this settings form is already WAY too crowded, so adding more functionality too it would squish everything even more. I wouldn't be for this, but happy for others to go for this.

Yeah adding good descriptions to some of these fields would be good, making them editable sounds like way too much effort for little gain.

Now we just need to come up with a description for UUID and somehow make that distinct from ID for non developers ;)

I think we made this search work at some point where you could add operators to your search terms like + to find exact word matches or quoting stuff to find sentences, but we didn't go that route because it got too complicated. Probably easier nowadays? That way you could search for +ID and just get the ID field

Added minimal test coverage, about the same as what we have for the other data that is rendered in the preview. Since different databases output very different explain data it seems, actually checking the output might be too much here since we might need to switch by database type, which I don't think we want to do, sounds fragile and excessive.

On SQLite this seems to output quite a lot of data, do we want to add a toggle for adding 'explain' data, like we have for the other data?

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

Is this a duplicate of 🐛 Views exposed filter reset creates session for anonymous Active or just similar?

Updates IS

Going through (really) old issues to clean up the queue.

Unless somebody comes up with a reason to change whatever we've been doing the last 10 years, I'm going to go out on a limb and say, we just keep doing what we've been doing until now ¯\_(ツ)_/¯

Going through (really) old issues to clean up the queue

Yup, --raw isn't that raw, its safe to use

Going through (really) old issues to clean up the queue.

Quite a lot of work was done on the other issue after this was opened, so not 100% clear if this is something that still needs to be done. The IS isn't super clear ;)

So going to close this for now, feel free to open this back up if there is still something we could improve here

Going through (really) old issues to clean up a bit.

The issue that this was based on was Closed (won't do), so just reading the IS here it seems that this won't need doing either.

Feel free to reopen this issue if you feel there is still something we can/should do here.

Going through (very) old issues to clean up the queue

This doesn't seem to be happening anymore? I imagine it got cleaned up somewhere in the past 10 years.

Feel free to reopen if I overlooked something and we still need to so something here.

Couple of fails seem unrelated, fixed the ones that were also failing locally

Going through (really) old issues to clean things up.

I'm going to close this as a duplicate of #1668024: Provide a unified user interface for static filters, exposed filters, contextual filters (arguments) and facets → even though it isn't really. But I don't think we will ever come up with a term that would make it clear to end users new to Drupal what the differences between these filters is (it's hard enough to explain to new devs), they should all just be "Filters" and that is what unifying them (hopefully) would give us

Feel free to reopen this if you come up with a great label!

Going through (really) old issue to see what can be cleaned up.

This is certainly still an issue with quite a bit of Views config, but the IS here, I feel, is lacking a bit of concrete actionable things. So I'm going to close this as a duplicate of #2917814: Views table style info never gets updated when changing/adding fields → which is a bit more concrete as to where to start.

Feel free to reopen this if you feel there is something concrete we can do here.

Going through (really) old issues to clean some thing up

I don't think we need to expose this information to the end user, it's developer information, and to get the correct information from a method we could expose to developers you would need to provide all the information except the prefix first.

If there is a use case where this information would be useful to an end user it might be useful to show somewhere, but closing for now, feel free to reopen this if anybody feels there is a good use case for this

Going through (very) old issues to clean up things.

Changing this still makes sense 10 years on, but might come with a bit more BC baggage than it did before.

So, the MR just changes the default value.

I would argue against doing much more here.
We shouldn't update existing Views on sites because that might break things.
Most Views will just ignore this, because if you are using a Table style this setting is actually ignored (this goes for all Views shipped by core that I could find).
We don't do tests for this normally, it's just a default setting getting changed, existing test for this might fail, lets see.

Only thing I could think of that might be sort of worth doing is setting this to false on all default Views that ship with core, so that the settings line up (even though they get ignored by all these Views anyway, so very limited use in that)

lendude → changed the visibility of the branch 1899220-remove-or-at to active.

lendude → changed the visibility of the branch 1899220-remove-or-at to hidden.

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

Going through (really) old issues to clean up the queue.

While I agree renderLink is indeed not great, buildLink still doesn't tell me what this method does, so to me that isn't worth changing this over. To me, the problem is that this method does two things. It updates settings to make sure we are going to be rendering a link and then returns a string to use in the link. So, to me, we should either split this up into two methods or make sure the name represents what it does: setRenderAsLinkAndGetLinkText() which feels very verbose but at least tells you what it does ¯\_(ツ)_/¯

Going though (really) old issues for some clean up. The request is now gotten from $this->view which sounds fine, so closing this

@pcambra I think everybody in maintainers.txt can change target branches in MRs now, so updated it to 11.x, but that still doesn't seem right looking at the changes. Is 11.x the correct branch to target?

I think the feature makes sense and is probably much harder to do in contrib than adding it to core since this stuff isn't exposed in any sort of hook.

I'm not sold on the names and descriptions used here. I'd use 'key' and not 'id' to describe these, but even than it's still pretty hard to grasp what these are for. Since this is some pretty specialised config, we need strong descriptions to make clear what these are for I feel.

Is this maybe trying to fix the same problem as #2632828: Block view cachability metadata doesn't get copied in BlockView::preRender() when the block content is non-empty → ? Not sure, but feel very similar.

@xjm 10.4.x seems to have been missed?

Nice, wasn't sure what the policy is nowadays with adding interfaces for everything, but this would allow us to decorate the file.upload_handler service again.

Looked at the MR, looks good, searched for other uses and this seems like everything, only some comments now reference the class directly, but those point to a protected method on the class so that seems correct.

Since FileUploadHandler doesn't have an interface and this plugin now hardcodes the used class, you can no longer decorate the file.upload_handler service and use this plugin without fatal errors ¯\_(ツ)_/¯

$this->options['content'] is expected to be an array but can sometimes be a string

@lance lancelot Have you been able to pin down when or why this would be the case? We still need steps to reproduce this on a fresh Drupal install since as it stands currently, the error might just be the correct way to handle this because there is something wrong upstream.

smustgrave → credited lendude → .

Didn't test, but what happens on a View with no pager and you try to use view.pager.current_page? Should it check for a pager before using this? And does the Some pager have view.pager.current_page?

Just looked at existing coverage that might make testing this easier, but alas, there seems to be very little coverage for the list style, just \Drupal\Tests\views\Kernel\Plugin\StyleHtmlListTest as far as I could quickly find. No tests for 'ol' list at all.

longwave → credited lendude → .

Tried to reproduce with the given steps in a clean Umami install but I can't, not much has changed around this code so I'd be surprised if it was suddenly fixed, but we need some new steps to reproduce I think

Discussed in #bugsmash with @longwave and @smustgrave and we agreed that dropping the 'Should' from the wording entirely would probably make this clearer, so something like "Use replacement tokens from the first row" and the last one "Display on empty results".

Other opinions?

Per #5, this is indeed not the change we are looking for, we need to properly deprecate this in the base handler, we can't just remove it

The IS is still talking about D7 and contrib View, where it looks like this has actually been addressed, so could somebody update the IS to make it more current? I'm assuming this is still an issue