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 review 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
This is probably caused by 🐛 Allow TimestampFormatter to show as a fully cacheable time difference with JS Fixed
This looks like a duplicate of 🐛 Dates are always wrapped in a HTML tag in a Views "REST Export" display Active , if this is addressing a different issue, please feel free to reopen this
Feels related to 🐛 Field options "Rewrite results" and "Link to the Content" applied together is not working as expected Needs work
We still get this after adding an empty selection option to the Select element through code, the fix in #2 still works.
Very obscure use case, so won't bother with a patch, but just leaving a comment for others that might still run into this after doing custom modifications on the field.
In my opinion, no, because we have no idea what we are 'fixing' here. But I'll leave it RTBC for others to look at, happy to be wrong :)
I agree with @smustgrave this is most likely just masking an issue. If you have a tab and no route there is something wrong with your install, so why are we suppressing the warning, when it seems to be doing it's job and warning you that there is something wrong?
And yeah it seems most likely a timing issue that $this->state->get('views.view_route_names') is not properly populated yet or something along those lines. But I would that if that is the case we need to fix the timing issue and not just ignore the error.
But I think we need to properly investigate the root cause and then we can make a decision as to the proper fix (which might turn out to be to suppress the error)
Wouldn't a filter setup like this also give 'wrong' results if the field value contains something like an HTML tag in the spot the user is searching? I don't think filtering on HTML containing fields not giving the results an end user might expect is really new, nor really a bug. The & thing might be new, but just a new symptom on an existing behaviour I think.
Views searches in the literal value in the database, that is by design. Putting a 'contains' filter on a full HTML text field has always been dubious and unperformant as far as I know.
Looks good, one more test case would be nice.
Can we come up with other cases that the regex might have missed like in #8 and make sure we are ok with that?
This is by design, Views will look for the first display with a path and use that. You can try #2 or make the block and page be separate Views and not 2 displays on the same View.
How would you get these 'wrong' URLs? Manually I would guess? Or is there some way that these are generated through Drupal?
Making this a little more robust makes sense, but doesn't feel like a bug unless Drupal generates these URLs sometimes.
Yes, timing is everything. Updating the pager after the query has run, doesn't do anything useful. As shown in #10, changing this before the pager is used is the way to do this.
Nice, this gets Views inline with the changes intended in 📌 Use a modal for entity delete operation links Fixed . Solution looks good, test coverage looks good (couple of unrelated code cleanup changes I guess, but ¯\_(ツ)_/¯)
The new approach makes sense to me too, follow up steps look good and make sense.
Left some minor nitpicks on the MR but this sounds like a great way to get something in without it being very disruptive.
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
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 →