$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 →
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
Hide patches
Colleague just ran into this, lets fix it. Moved #30 to a MR
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
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.