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 ¯\_(ツ)_/¯
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
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
#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 → .
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?
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.
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
Nice find, change looks correct. Didn't find any other occurrences of this in Views so scope seems good.
Per #3 this needs steps to reproduce with (preferably) just Drupal core, usually changes like this are just masking upstream issues and you actually want this to give an error because something upstream is providing the wrong thing.
Not sure if this is a bug, think this is pretty much by design, so I'd say a feature request, but ¯\_(ツ)_/¯
Caching the View might become tricky cause that would potentially depend on permissions depending what the rewrite would lead to.
Would still be good to get some steps to reproduce this on a clean install of Drupal core to make sure this is a core issue. The steps in the IS by themselves are certainly not enough to trigger this on a clean install
#263 breaks for us when a block ID is found but a Broken block is loaded in $block_instance = $this->blockManager->createInstance($block_id);, so added hardening against that
In my opinion we shouldn't provide a fallback field, it's not like we provide one for any of the other fields, right? So I would opt to keep this as simple as possible and just stick to one field
Just my opinion, so feel free to ignore, but consider this reviewed by a subsystem maintainer
Added the proposed code and a test assertion. I can see this happen when testing manually, but the test isn't catching it.
The assertion detects the session when I log a user in a test, so that part works, but in the test, the reset is not triggering the bug. Haven't figured out why yet
If you look at the code in \Drupal\views\Plugin\views\field\EntityField::getValue you can see that there is some extra logic needed to get the correct value, mainly getting the right translation for the requested data. So we can't just grab the value from the row, because that would lead to unexpected values being shown.
Bu, yeah, the docs on that are a bit unclear as to the exact purpose of this method. But I don't think we can really change the way it works without breaking a lot of other things.
See the comment in \Drupal\views\Plugin\views\field\FieldLanguage
// No point in displaying the language field on monolingual sites,
// as only one language value is available.
This is by design.
I'm not sure what will break if we don't add this, but probably something
This feels very familiar but can't find the issue this makes we think of.
The described use case sounds VERY niche, if this is the only way to trigger this, it will probably require a very focussed fix in order not to create any regressions.
Does it also serialize booleans as strings when you don't rewrite to 0/1? With the rewrite would you expect an integer and not a boolean or a string?
Or can you not render the View in a render context? The proposed change sounds like it might break some things (haven't tried).
@codebymikey I see you already tagged 🐛 Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page Needs work as related, can you say why this is a different issue, feels duplicate to me at first glance.
Solution and test code look good, could maybe check for the reply link to exist and not just the text to be a little more specific but this works.
my custom views (not the module)
What does that mean? Are they Views from the Views module or are they database view tables?
On a clean Umami install (which contains english and spanish out of the box) I go to /admin/config/development/configuration/full/export and then export the config it contains a 'es' folder that contains all the spanish config, including the Views.
Not sure what you are looking for, that config should just be exported along with the other config.
I tried to reproduce this in a clean Umami install and couldn't.
- Added a View with two block displays filtered on different tags both showing recipes
- Added both blocks to a Basic Page
- Viewed the page and it showed both blocks (logged in and anon)
Can you reproduce this on a clean Drupal install or only on an specific install?
Hmm never mind, that needs to be fixed in views_data_export. But the combo seems to be broken now
As indicated by the automated test fail, the module still uses a removed method getCellByColumnAndRow()
This leads to a fatal error, so this call needs to be replace per https://github.com/PHPOffice/PhpSpreadsheet/issues/3930
There was a fatal error when submitting an empty value. Fixed the error and added some test assertions to check this scenario.
This still needs automated test coverage before it can be committed.
Can you reproduce this with just core?
I tried and couldn't with the provided information. What I tried:
* On a clean Umami install
* Go to the content view
* Duplicate the page display and give the new display a new route
* Save the View
* Disable the new display
* Save the View
* Click around the View UI => no problems
So not sure what the issue is, but it's not just having a disabled View with a route, it seems to need more to break