🇬🇧United Kingdom @steven jones

I've opened a MR that takes the patch from #22 and then fixes a couple of bugs.

Seems to work though!

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

The version in the MR builds the same sort of single SQL query as your proposed NOT EXISTS query, does it not? It’s a single, nested query, not two queries.

I’ve not put both statements through a query planner but would be surprised if they don’t basically boil down to doing the same thing.

The MR has this already: https://git.drupalcode.org/project/commerce/-/merge_requests/163/diffs?c... I reverted it, but I suppose it could be kept and wrapped in a flag as you suggest.

In case someone else wants it, I've added a patch that simply removes the router item so that you can't get to the form and break your own site.

Just got bitten by this one, I feel like config has a better way to do these sorts of temporary overrides.

Although the solution proposed in the MR is better than the current implementation it does mean that while the email is being sent any other emails sent by other requests will also use the potentially changed mailsystem settings too.
This should be isolated by using some kind of config.factory.override tagged service that'll toggle the config to what the user selected without it being saved to the persistent config storage.

Okay, this looks good, it seems we do have some tests for composite elements and states already: test_states_server_comp does seem to do a bit of this, and my new code is causing that to fail, which makes sense because I've left the bits that aren't working as TODOs, because yeah...they are hard/no idea how we're going to do that.

I still think we need an additional test that'll cover the case of a composite element on the second page of a form, where it's a required field, but won't currently be marked as such because it'll depend on the state of an element on a previous page. I'll work on getting such a test written up tomorrow, because yeah, the webform tests are seriously daunting if you've never worked on them before and it's taken me a few hours to get to grips with how it's all done.

I've gone ahead and pushed some code. It's definitely not complete, and I took the liberty of copy/pasting some code rather that introducing a helper method as described in #4 because for now, it might be useful to see where we need to introduce changes to get it to work properly.
It doesn't look as simple as letting stuff run on a composite element, because those elements aren't actually there at the point this code runs.
Once we've got the code working, I reckon we can clean it up to factor out common elements between the two code paths.

We're missing some tests for sure.

This code is currently working for my very simple use-case as described in the issue summary, so we should be able to construct a test that will then pass with this code.

I think there's a danger of all the edges cases though. So for example, at the moment I've skipped some of the copy/pasted code that would set an element to be visually hidden, because the elements aren't there to visually hide! But that might actually be okay, because if a wrapper is visually hidden, then that'll probably have the same effect.

Also, there's a todo about an after build that's getting added, maybe in the right way, I'm not sure yet!

I'm going to unassign myself, but might still come back around to look at getting a test sorted out at least.

Going to have a look at this issue.

Patch in #10 / MR!36 is working for us, thanks for the work here.

Let's focus on MR!6 and let's use the attributes collection to pass in the title, rather than adding a new attribute directly in the template.

steven jones → changed the visibility of the branch 3244450-title-export-icons-with-option to hidden.

steven jones → changed the visibility of the branch 3244450-title-export-icons to hidden.

YES!!! This would be so cool! Let's get the MR up to date.

I converted the patch in #2 into MR!71

Let's try to make that cleanup I mentioned in #4 and add some tests.

Oh nice! This would be a great little feature I reckon.

+++ b/templates/export-icon.html.twig
@@ -12,6 +13,6 @@
-<a href="{{ url }}"{{ attributes.addClass('feed-icon') }}>
+<a target="{{ target }}" href="{{ url }}"{{ attributes.addClass('feed-icon') }}>

I wonder if we can get the target into the attributes here?

Does this need a test to make sure that the link is getting generated with the appropriate target?

This got solved in 🐛 Admin theme not active when using batch export method and a display path starting with admin/ Needs review I reckon. Thanks everyone for the work here.

We have 📌 Automated Drupal 11 compatibility fixes for views_data_export RTBC so I don't think we need this issue.

As mentioned over in ✨ Add Western Europe ISO-8859-15 encoding to csv export data Postponed we need this feature to land here first before we can offer the UI to select an encoding.

I suppose it would be nice if there was a way to get a list of valid encodings that we could present a UI for.

Also, you should decide if you want to accept utf8 or utf-8 since the code at the moment sort of uses both.

I think we need to wait for this feature to be in the CSV Serialization module before we can implement the UI to allow the selection.

✨ Add support for more encodings Needs work

We'll wait to see what version of utf8 or utf-8 gets into the upstream module (at the moment it's using both) and then we'll adjust our code accordingly, and we can collect together all the great effort here on tests, update hooks etc.

Thanks for you efforts here, but it looks like this code has changed a lot since 5 years ago, and this isn't needed any longer.

Thanks everyone!

@ericgsmith thanks so much for the issue, code and the tests, super helpful.

I've basically committed what you had, but in our method that generates the original route, rather than altering the collection of them. Seems to work fine, and your tests still pass :)

Looking at this now, we already provide the route and change some options on it, so we don't need to alter our own route I don't think.

I'm going to put this back into Needs work, and I'm going to assign it to myself, I'll work on it and get it to a place where I'm confortable committing it, ideally to 1.x, but maybe it'll go in a 2.x version.

I'm not going to commit this as-is because I don't think that the data export class should be making calls like drush_backend_batch_process if it can help it. Like, it seems weird that we're setting this 'your running in Drush' option and then changing quite a bit of the exection if that's the case. I'd rather find a way to isolate all of that to the Drush command if possible, providing enough output/hooks/events where needed for the Drush command to do what it needs to do.

I don't anticipate the actual invocation of the Drush command or the output changing between now and the final code that gets committed, so everyone here who's using the patch should be able to keep on using it and then do a trivial removal of the patch when it lands in a stable release of VDE.

Thanks for the patience everyone, should only be a little longer.

@csmdgl I'm not sure it's a bug, because in Drupal 7 it's actually the case that you can send a scalar or an array to a database condition and Drupal would pick out the right operator:

https://api.drupal.org/api/drupal/includes%21database%21query.inc/functi...

I'm going to close this issue down because Drupal 7 is EOL and I suspect that others have found ways around this in the meantime anyway.

Thanks for the effort everyone though.

I applied the initial commit from MR!5 and that worked fine for my use-case on a simple Drupal 11 site.
Spaceless isn't gone in Drupal 11 afaik, it's merely deprecated, so it's probably optional if the maintainers want to commit that bit or not.

steven jones → created an issue.

We removed this code in 2.1.0-rc1.

Thanks everyone for the work on providing code and testing, much appreciated.

Made the release: https://www.drupal.org/project/content_access/releases/2.1.0-rc1 →

Okay, that seems to still work, I'd rather get the fatal error if it is coded wrong and the bug reports than silently ignoring it at runtime.

2.1.0-rc1 here we come.

Spotted that we've got a function exists call that might be masking something, committed removing that and will see if the tests still pass, if they do then I think we can cut a 2.1.0-rc1 release.

All tests are passing in Drupal 10 and 11!

steven jones → created an issue.

ACL just got a stable release for Drupal 11, so we should be able to progress this properly now.

Let's go back to reviewing the MR.

We should leave this as RTBC so that we get further updates posted from the bot.

Yeah, that's a good shout @greggles let's reduce the scope of this issue to get the PHPCS report enabled, and we can simply let it fail for MRs while we work to reduce the fails in follow-up issues.

Thanks for the work from others here.

Going to commit this change, given the trivial nature, but I think this should be solved by imageapi_optimize I reckon, and I think it should make sure that the permissions are correct on a file, post optimization.

Thanks for moving this issue, among others along @greggles!

This is indeed a bug in Drush/core. Raised with Drush as: https://github.com/drush-ops/drush/issues/6238

Closing this issue as a duplicate.

Ahah! We're doing the right thing, but the html_title module was not, it got fixed 3 years ago in #3215926: A stray renderRoot() invocation is causing bubbling of attached assets to break → so closing out as a duplicate of that issue.

Ah sorry, Drupal 7 isn't supported any longer, so closing this issue.

I feel like Drupal should be able to generate a link with the destination parameter set? It does this a lot for other links etc. and I think that would be better than fetching the referrer header, which isn't even set by some browsers.

Is there a reason I'm missing why we're not using the destination parameter?

Also, we need some tests for these changes.

Added the gitignore.

steven jones → created an issue.

There's actually a method on \Drupal\views\ViewExecutable that tells us if there's a URL, I wonder if that's what we need to check here?

@dieterholvoet are you able to test this MR I'm about to open please?

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

✨ Create event once a data export finishes Active has a great idea: introduce an event that is emitted on export. We could get that issue in and then re-work this one to be an example implementation in a submodule.

This is very similar to ✨ Log data export event Active and yeah, I wonder if the best approach would be to get this working and merged in, and then re-work that issue so that it's a simple example implementation/submodule.

Thanks, the config schema issues raised in my original OP have been sorted.

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

Yeah, let's merge it.

Yeah, if we're going to allow logging then we should add some sense of what was exported, and to where.

I assume people want this sort of thing so that they can audit people exporting data from the site and keep an eye on them etc.

Fixing the title of this issue, because presumably we do all the query to be altered, just that there's something about the query metadata that's not quite right.

I 100% agree with #28 in that we should absolutely have a test for this.

I guess we need a module that implements a hook_query_alter based on some metadata that doesn't get currently passed correctly by the existing code.

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

Thanks for your response, but I think you've managed to research the Database Schema, rather than the Config Schema I think?

I searched for a relevant example in Drupal core, and I think the most comparable thing to a big lump of text is the request path condition plugin in the block system, i.e. this:

The config schema for that plugin is this:

condition.plugin.request_path:
  type: condition.plugin
  mapping:
    pages:
      type: string

I.e. a string, like I'm suggesting you should use too.

The main trait of the text type is that it specifies that the content is translatable, which is weird for this use-case.

So, I think it should be string.

In theory, if you wanted to change the data model, then you could store the data as an array of durations, rather than a string that gets exploded/imploded etc. but that's probably another issue :)

@goodboy I don't think the 'string' type imposes a length limit, though might be wrong about that! What makes you think it has a length limit?

@goodboy the naming looks better, thanks. Looks like the type of the expiry_durations property isn't correct, I think it should be 'string' not a 'text' because it's not translateable?

Woah! https://git.drupalcode.org/project/advban/-/commit/acee31b11c1aa9be5dc39... is going to break all existing installs, if you want to rename your config keys like that you're going to need to provide a migration path for existing installs.

I'd was advocating for changing the contents of advban.schema.yml to match the existing names.

steven jones → created an issue.

And yet if you do a Google search you'll find plenty of people asking to remove the 'blank' line from the end of their CSV.

I suspect it would be highly dependant on the reader of the CSV if a new line was needed at the end.

I'll mark this as a feature request, and do please add details of a CSV reading library that fails to read a CSV file if there's no newline character at the end of the file, thanks!

@averagejoe3000 thanks for the work here.

I suppose that we should add a test to ensure that we don't show the message to someone who can edit the settings, but not rebuild the access permissions.

Tests are still passing then, I'm inclined to merge this.

@Liam Morland should CSVs end with a newline character?

I've seen arguments for and against adding a newline character to the end of a row, making me think this should be a feature, not a bug? Do you have an example of why we'd want to add a newline character to the last row?

Ah @vasyok the storage of the file on disk in any specific location should really be consider a side-effect of running the export.

If you want to run the export and capture the contents to some other file on disk, then you either need another process doing the saving or get involved in ✨ Support views data export using drush Needs review .

I'm going to close this issue because batched exports are working fine for at least me and the automated tests, but feel free to re-open the ticket if the batch export isn't working for people on this ticket, when using it via the current API: i.e. clicking links in the browser.

Let's see what happens with this change.

I wonder why we need to generate an absolute URL? Drupal used to do that, but these days would typically do it via a root-relative URL. I wonder if changing over to that would be simpler.

Thanks for the bug report. Please search the issue queue before filling new issues. This issue is a duplicate of ✨ Add option to force https on redirect url Active

steven jones → changed the visibility of the branch 8.x-2.x-with-issue-3396154 to hidden.

steven jones → changed the visibility of the branch 8.x-2.x to hidden.

Merge request !29 now contains a 'flag' version whereby it's super quick to install the module, but you have to a one-time process in the webUI to build up the index table.

Does this need a Drush command maybe?

I've applied the patch from #14 to a 3.x-dev version and I'll open a MR shortly with those changes.

From my testing if you enable the module through the web UI you get a nice progressbar and batch process for building the index.
If you install via Drush then you don't get a progressbar, but it does work, and is using the batch to build up the table, nice! No timeouts etc.

I suppose that for environments that would timeout that Drush command, it's still not a great experience tbh. maybe we should move to setting some kind of 'rebuild' flag on module install, and then detect that and clear it when rebuilding etc.

I'm evaluating this module for a feature that I need to implement for a site, and so this might be a bit of a 'drive-by' contribution if I decide to not really use it, but I would like to commend the approach in #14 to use the Batch API.

Can I suggest that the approach this module has of maintaining a separate index table of data feels a lot like the node access API (or maybe Search API) and so can I suggest that you should be inspired by those systems. In particular I'd recommend switching from even trying to do the work on install of the module and instead make a robust batch API or queue process that does the indexing needed. Then on install pop a message up informing users that there's something else they need to do (unless there are no aliases in the DB) and then also a hook_requirements message that also informs administrators.

Also the patch in #14 doesn't apply to the latest 3.1.0 as far as I could tell, so this probably Needs Work.

Thanks everyone for your work on this issue, I've merged the MR and given liberal amounts of issue credit, thanks!

Setting to RTBC as before.

MR !384 fixes the issue for us nicely. Marking as RTBC.

This looks great, and thanks for the hard work everyone, but this sort of change really should have some tests to make sure we're not breaking things for existing sites, and that the new settings work correctly etc. so setting back to needs work.

I had this issue on a Drupal 7 site that was sort of close to going over the limit, the following code sorted it out:

  db_add_index('queue', 'temp', ['item_id']);
  db_drop_primary_key('queue');
  db_change_field('queue', 'item_id', 'item_id', array(
    'type' => 'serial',
    'size' => 'big',
    'unsigned' => TRUE,
    'not null' => TRUE,
    'description' => 'Primary Key: Unique item ID.',
  ), [
    'primary key' => ['item_id'],
  ]);
  db_drop_index('queue', 'temp');

I imagine this sort of approach could be adapted very easily for Drupal 11.

@chetananemade thanks for the updated patch. We should try to get this into a MR instead of using patches.

Yeah, field templates do not apply to the exports that VDE is doing, this as as-designed.
Use the hooks instead as suggested in #5

I would strongly suspect that as explained in #2 there is something else not quite working with your Drupal install.

Given the time since this issue was opened, I'm going to close it down.

Hmmm...I don't think it's correct that we ever allow specifying a view mode, so yeah, we should clean this up and not allow selecting one.
I wonder if you clone another display that uses an entity view mode, does VDE secretly store that and not allow you to remove it?

Hmm...is this an issue with our string, or the export that Drush produces?

Yeah, so at the moment there's no interface for us to check to see if this method is there, see: 🐛 Views style plugins with the `attachTo` method should implement an interface Needs work

But until then we can probably do a simple method_exists check and that'll stop the WSOD you were seeing.

Hmm...I suppose that it's nice to make this opt-in and won't break any existing export flows, but is there a reason to make this optional? Wouldn't you always want VDE to reflect the sorting options that the user has used?

Either way, this could do with some tests.

Closing this as a duplicate of 🐛 JSON is invalid when multiple batches are needed Active

As per 🐛 Batch XML export with multiple root nodes Closed: duplicate this is an issue for XML too.

I don't think we must have solved this issue long ago, so closing this one out.

Let's open a new MR with PHP CS enabled.