🇬🇧United Kingdom @steven jones

nicxvan → credited steven jones → .

@mcdruid so in all honesty I've not opt-ed in because I suspect that there are these sorts of security issues kicking around in this (sub-)module, obviously lets fix this issue but I probably need to get a decent security review of this module done, and then opt-in, otherwise it's going to be lots of issues being managed in private I suspect, which is bad for getting them right / being able to use the drupal.org tools etc.

I wonder if there are subtle security risks like placing a script in a location that this module is going to try to call, but doing something weird when the webserver calls it. Hmm...I feel like the security boundary needs tightly definining etc. otherwise it's going to be a nightmare!

Yeah, totally agree.

steven jones → created an issue.

@lendude thanks, but I think the comment in #6 was saying that the MR already fixed the container case, so we don't want to special case it, but instead revert it to what it was doing before this regression.

I've tweaked the code back to the special casing of only the password confirm and item elements, as per the previous code that was in core for some time (plus handling the password confirm case non-ideally).

Once we've gotten this regression sorted, and the test cases in to verify that, then we can discuss changing the behavior / implementation, which didn't really happen in 🐛 password and password_confirm children do not pick up #states or #attributes Active which is what was causing the whole problem in the first place.

@tim.plunkett I assume that adding the container test is enough here to get this regression committed and fixed?
I mean, technically, we're testing the else case of a conditional, so we could add a lot more test cases, but that seems redundant, we'd essentially need an infinite set of test cases to double check that Drupal does the 'right thing' when A != B, for all values of B that don't equal A.
For clarity we have this conditional:

$key = ($elements['#type'] == 'item' || $elements['#type'] == 'password_confirm') ? '#wrapper_attributes' : '#attributes';

And our tests send that code cases where: $elements['#type'] is:

• 'date'
• 'item'
• 'password_confirm'
• 'container'

steven jones → created an issue.

steven jones → created an issue.

Going to credit users who contributed to ✨ Changing default "" and "" tags in XML export Needs review relating to encoding.

Sorry about the noise there, was on a machine without access to running PHP unit!

Anyway, I think this is ready to be committed now.

Is there any particular advantage to changing this restriction, Drupal 12 isn't out until the middle of next year, and who knows if VDE will be compatible?

I've added the start of this. Still need a post update hook that'll update all existing views to set their encoding to the empty option, which will then preserve what they're doing already.

Thanks everyone for your patience, I'll note that I'm committing a partial version of what was in MR!75, so if you're using all the features of this MR/patch then you might want to stick with that until all the sub issues of ✨ [meta]Make XML output more configurable Active are in, and then upgrade to the version of VDE that has those in.

Lets see if we can set the encoding to be enabled by default for new installs, and otherwise keep things as they are for everyone until 8.x-2.x.

@joelpittet yeah, agreed, I think it should almost always be in there, but I don't want to break something random in someone's export, so I think I'll punt that change to ✨ Allow specifying the XML encoding property Active and maybe we can make it the default for new installs or something like that.

You happy with the code otherwise?

steven jones → created an issue.

Sort of tempted to remove the cheeky addition of the encoding="utf-8" bit, and then the changes here really will be absolutely minimal and could get merged right in.

steven jones → created an issue.

steven jones → created an issue.

steven jones → created an issue.

steven jones → created an issue.

steven jones → created an issue.

steven jones → created an issue.

I don't particularly see an explicit dependency from Views Data Export to phpoffice/phpspreadsheet, where is that?

I can see that we are indeed calling some phpoffice/phpspreadsheet code, are you saying that the API has changed upstream and we need to sort VDE out?

Can you outline what changes you'd like to see VDE make or outline the errors you're seeing please?

Thanks @joelpittet for the work on this, and thanks everyone else for your continued patience with Views Data Export maintership!

Okay, so the code as it stands does have a few issues, specifically because of using string replacements to do some of the heavy lifting rather than an XML library. This reminds me of the famous stack overflow answer about parsing HTML with regex.

For example, at the moment I can set both the 'root node name' and 'item node name' to say 'response' and then use the new 'XML metadata field to add some more stuff in, and all that stuff actually gets repeated throughout the file, not only after the root node, as is the intention of the code.

I think there are a lot of sticky issues that we might need to sort out with this stuff, like the fact that it seems weird that you can inject arbirary non-valid XML into the middle of the XML document anyway. Seems like this is going to be super brittle going forward if this doesn't get done right.

I appreciate that the current batched export code also does string replacements, but I think that's another bug waiting to happen. Presumably you can't have a field called 'response' in a batch XML export, yes in fact, that breaks very badly at the moment!

I think what I'd like to do is:

1. Restrict the scope of this issue to the original report only, i.e. allowing someone to change the <response> and <item> tags.
2. Work on getting a test in for that functionality, which should be pretty easy tbh.
3. Get just those points committed and mark this ticket as done. This will give us a proper place to put options, the beginnings of the code that's currently in the MR anyway and add in custom handling for XML where we'll need it in the future anyway.
4. Replace out the current string manipulation of XML in the batched export with proper XML manipulations. This can be a new ticket that could be worked on separately.
5. Raise separate tickets for the other features introduced in this ticket, which is:
   • Removing the root node entirely (presumably only valid to do if there's only a single child node anyway.)
   • Adding attributes to the root node - This doesn't need to be done as a single text field, as the Symfony serializer actually has built-in support for doing these already, so we could provide nice escaping etc. and not do broken plain string swaps.
   • XML Header (the metadata) This seems like a bit of a weird one to me, but I understand that there might be some use-cases out there that would want to add some arbitrary XML to the root node of the file. Again this should get added via the XML encoder if possible, but maybe having big warning messages all over this entry field that you're likely to break things unless you know exactly what you're doing, then maybe it's okay.
   • Removing empty values from the output.
   • Adding an extra wrapper with an arbitrary name.
6. Encourage others to work on those tickets/get them committed in a timely way.

Sorry if that sounds super annoying / convoluted, but I think rather than hold up some of these features because they aren't all ready, it makes sense to split them out.

I'll make the other tickets in moment, and we can refocus the work here to be back to the original issue.

@memtkmcc cool, so it seems like there's a consensus to mark the projects as maintained at some level, by Omega8cc, that works for me.
Thanks!

Are the fixes/patches done for BOA being contributed back onto Drupal.org at the moment? Or are they elsewhere?

I'm concerned that people out there who are expecting their Aegir installs to continue indefinitely aren't going to understand that they'll be moving to a different system: BOA to continue to be able to get maintenance updates etc.

For example...Aegir supports Apache, but last time I looked BOA was nginx only, right? So we'd need to make sure people aren't using Apache features etc. if they want to move to BOA.

steven jones → created an issue.

Okay, makes sense. I guess we'd need to make sure that it was clearly stated that it wasn't a continuation of the relatively unopinionated Aegir project, but was BOA, which last time I looked was far more opinionated?

@memtkmcc that sounds great to me, presumably in that case you'd want to keep maintaining things on github, and essentially change the drupal.org pages to point to those projects, similar to: https://www.drupal.org/project/drush → ?

Probably want to wait for anyone else to weigh in before going down that road though.

steven jones → created an issue.

@jhedstrom thanks for the MR I feel like this is the sort of thing that really should have a test. We have a test for CSV exports, but not for XLS ones, but we do include the module for testing etc. We should probably have a test for this sort of thing so that we don't introduce regressions etc.

I think we could either add another test case to the existing class, or have a new class for testing XLS stuff specifically, don't mind.

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.