Account created on 25 December 2006, almost 19 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom steven jones

We're going to fix this properly in Make the export buttons 'better' Active so that you can do this sort of thing.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

Merged.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

Changed my mind on that last one. Trying to get some velocity!

🇬🇧United Kingdom steven jones

Amazing! Thanks so much.

🇬🇧United Kingdom steven jones

I was waiting on an LLM to give me a image to use, which is now has, not perfect, but decent enough.

🇬🇧United Kingdom steven jones

I've changed slightly the implementation and taken out the custom configuration, I'm not sure we need to have another title, do we?

The patches here had it using an attributes array, and the title was in that, not sure if the approach with moving the title in a preprocess is better/worse, opinions?

@svendecabooter would that suffice for you? You mentioned that the title was being set by contextual filters? But presumably that's optional? You could turn that feature off, can't you?

🇬🇧United Kingdom steven jones

Done. Thanks @kreynen for the issue and the clear path to resolution.

🇬🇧United Kingdom steven jones

Let's do what Search API did I reckon.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

Urgh. Drush.

🇬🇧United Kingdom steven jones

Feature requests should go against the latest dev version.

And I've updated the MR, will try to get this one merged this week.

🇬🇧United Kingdom steven jones

@eelkeblok thanks for spotting the issue and the code to fix it.

🇬🇧United Kingdom steven jones

Gah, spoke too soon.

🇬🇧United Kingdom steven jones

Okay, I think we're good to try and release 1.7!

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

Okay, so in 🐛 Improve cloning in views to prevent crashes. Active I've added some code that will validate the display as having valid style and row plugins, so that'll avoid users getting into this sort of situation I believe.

If some migration framework is simply cobbling together some random YAML and hoping that it'll work in Views, then that's the thing that needs fixing really. You could break lots of Drupal by playing around with config at a very low level. And there's plenty of code that'll break etc.

At least now, if you were to edit the view you'd get some helpful messaging.

I'll raise a ticket against core, explaining the issue etc. and maybe it'll get sorted and in.

I think I'll mark this ticket as a 'won't fix' because I'm not sure there's much we can do here.

🇬🇧United Kingdom steven jones

Committed the fixes.

🇬🇧United Kingdom steven jones

Almost there!

🇬🇧United Kingdom steven jones

Thanks everyone, committed!

🇬🇧United Kingdom steven jones

Yeah, Drupal core doesn't bother to validate that a selected style plugin is valid.

I've got some code in https://git.drupalcode.org/project/views_data_export/-/merge_requests/90 that does, and that at least stops you from creating new views with this issue via the views UI. Not sure if that method gets called at other points to stop views that have made it to the config system already, and are incorrect...but honestly you could break lots of things like that.

I think that the fix over there will cover off the most common way to get to this error, and we can work on 'migration' support, and we can also raise the core bug and let that one sail off into the 'never fixed' ether.

🇬🇧United Kingdom steven jones

Right, I think this will work. We should add some tests though.

We are now validating the view configuration using the validate method.

Ideally views would do this for us, maybe we'll work to get this method into Drupal core...but that's core dev, so...yeah, might take a while!

🇬🇧United Kingdom steven jones

Urgh, this really is a core bug, as you can reproduce it with a plain Drupal 11.2 site.

Steps:

  1. Create a new view
  2. Add a page display, and override the default display plugin, but set it to the 'default' plugin, which is called 'Unformatted' in the UI.
  3. Use the 'Duplicate as REST export' functionality to produce another display
  4. Tweak the path of this display so that you can save the view
  5. Save the view
  6. On collecting routes, this will now cause a fatal error, since the Rest Export display plugin assumes that the style plugin is compatible, but it isn't because it's the 'default' plugin and doesn't have a getFormats method.

This is actually related to another VDE issue: 🐛 Improve cloning in views to prevent crashes. Active

🇬🇧United Kingdom steven jones

I don't think the initDisplay method is the right place to do this, is it? Is there a method that gets called when cloning a view?

🇬🇧United Kingdom steven jones

Thanks everyone!

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

I've changed it to:

Image Optimize module is a utility that provides a way for site builders to define <em>pipelines</em> that will process images to reduce their file-size while hopefully still maintaining image quality.

This helps you to get better Google PageSpeed scores, because you'll be sending fewer bytes over the wire to your users.

Pipelines can be applied to images in different ways, and can be configured to do different things to different images.

We provide built-in support for core Image styles and a service allowing use in third party modules. With a few clicks you can optimize all the images on your site being rendered via Drupal's image styles.

There are extension modules that provide <em>processors</em> that operate on a images. To reduce their filesize. Some of the processors send your images to a third-party optimization service, some do the processing locally for privacy and/or cost reasons.

 At least one or more processor module needs to be installed for the optimization pipeline to do any meaningful work:

<ul>
<li><a href="https://www.drupal.org/project/imageapi_optimize_binaries">Image Optimize Binaries</a> (for binaries locally installed on the server)</li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_resmushit">Image Optimize reSmush.it</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_tinypng">Image Optimize TinyPNG</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_gd">ImageAPI Optimize GD</a> for adjust compression quality per style</li>
<li><a href="https://www.drupal.org/project/kraken">Kraken</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_avif_webp">ImageAPI Optimize AVIF & WebP</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_webp">ImageAPI Optimize WebP</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_avif">ImageAPI Optimize AVIF</a></li>
<li>Know of another module: <a href="https://www.drupal.org/node/add/project-issue/imageapi_optimize">raise an issue</a> and let us know!
</ul>

<a href="https://www.drupal.org/project/issues/imageapi_optimize?version=All&text=&status=Open&priorities=All&categories=All&component=All&order=field_issue_priority&sort=desc">Open issues</a> | <a href="https://www.drupal.org/project/issues/search?projects=Image+Optimize%2C+Image+Optimize+Binaries%2C+Image+Optimize+reSmush.it%2C+Image+Optimize+TinyPNG&project_issue_followers=&status%5B%5D=Open&issue_tags_op=%3D">All issues from all 'official' subprojects</a>
🇬🇧United Kingdom steven jones

Yeah, on it.

Here's the old version of the project page for posterity:

<h2>Drupal 8+</h2>
A Drupal 8+ version of this module is in active development, a relatively stable beta version is available.
The Drupal 8+ version has been completely re-written and no longer provides an image toolkit. Optimizations are now defined as 'pipelines' that can be applied to images in various ways.
The module has built in support for core Image styles and a service allowing use in third party modules.

The module has been split into several other projects for easier maintainability and more modular installing. At least one or more processor module needs to be installed for the optimization to work.

<ul>
<li><a href="https://www.drupal.org/project/imageapi_optimize_binaries">Image Optimize Binaries</a> (for binaries locally installed on the server)</li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_resmushit">Image Optimize reSmush.it</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_tinypng">Image Optimize TinyPNG</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_gd">ImageAPI Optimize GD</a> for adjust compression quality per style</li>
<li><a href="https://www.drupal.org/project/kraken">Kraken</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_avif_webp">ImageAPI Optimize AVIF & WebP</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_webp">ImageAPI Optimize WebP</a></li>
<li><a href="https://www.drupal.org/project/imageapi_optimize_avif">ImageAPI Optimize AVIF</a></li>
</ul>

<a href="https://www.drupal.org/project/issues/imageapi_optimize?version=8.x&text=&status=Open&priorities=All&categories=All&component=All&order=field_issue_priority&sort=desc">Open Drupal 8 issues</a> | <a href="https://www.drupal.org/project/issues/search?projects=Image+Optimize+%28or+ImageAPI+Optimize%29%2C+Image+Optimize+Binaries%2C+Image+Optimize+reSmush.it%2C+Image+Optimize+TinyPNG&project_issue_followers=&status%5B%5D=Open&issue_tags_op=%3D">All issues from all subprojects</a>

<h3>Drupal 7</h3>

This is a toolkit for <a href="http://drupal.org/project/imageapi">ImageAPI</a>. It requires imageapi_gd or imageapi_imagemagick or any ImageAPI toolkit to work.

ImageAPI Optimize allows you to use your preferred toolkit and optimize (losslessly) the image when it is saved. Practice for web performance suggests that images should be optimized for better loading time. With this module enabled, Google's Page Speed will always give you an A in image optimize.
<!--break-->
This module supports 3rd services (like Yahoo! SmushIt, since 1.5) or many tools if you can compile them on your server. Read the <a href="http://drupal.org/node/773342">documentation page</a> for a full list of supported tools.

The size reduction varies on the original image, normally between 10-30 KB if image is not optimized before. All optimizations are lossless, but metadata is removed.

This module was featured on <a href="https://www.lullabot.com/articles/module-monday-imageapi-optimize">Lullabot's Module Monday</a>.

<h3>Features</h3>
<ul>
  <li>Lossless optimization</li>
  <li>Works with any toolkit (GD or Imagemagick)</li>
  <li>Pluggable optimization tools</li>
</ul>

<h3>Quick install instruction</h3>
- Download imageapi_optimize (and imageapi, if you don't have it yet), enable it.
- Go to ImageAPI settings page (admin/settings/imageapi), select ImageAPI Optimize as the default toolkit instead of GD or Imagemagick
- Go to ImageAPI Optimize settings page, select SmushIt if you haven't other tools compiled.

Despite the name, this module does not depend on ImageAPI. It depends only on the core image.module. Please read the documentation page for more information.

If you prefer Image Magick over GD, please use <a href="http://drupal.org/project/imagemagick">ImageMagick</a> module instead of ImageAPI (D7).
🇬🇧United Kingdom steven jones

Yeah, I think we can work toward that.

🇬🇧United Kingdom steven jones

The first time I looked at the code was yesterday! But to my eyes the order does need changing like I've proposed in the MR.

🇬🇧United Kingdom steven jones

Tests are failing with this fix, but also, I'd be tempted to say that the fix isn't really addressing the general problem, which is that the options are being merged in the wrong order really.

At the moment the code is:

$parameters = NestedArray::mergeDeep($additional_parameters, $custom_parameters, $module_parameters, $geolocation_parameters);

But that means that the base parameters always win, with no possibility to change that.

Seems like they should at least be:

$parameters = NestedArray::mergeDeep($geolocation_parameters, $additional_parameters, $custom_parameters, $module_parameters);

And even then, there would be an argument for doing:

$parameters = NestedArray::mergeDeep($geolocation_parameters, $custom_parameters, $module_parameters, $additional_parameters);

I think?

🇬🇧United Kingdom steven jones

@pingwin4eg thank you for the patch in #167, that solved my issue:

I have a table of line items view, and I added a left join on the products, so that I could display some fields from a product if the line item referenced one, but that removed the line items which didn't reference a product.
Your patch allowed the rows that didn't left join a product in to be allowed again, and thus appear in the table.

From #150 @rszrama seems to be suggesting that nothing is going to get committed here, rather we'll document lots of use-cases and tradeoffs, and let people discover this issue and work out an appropriate fix for their site.
So is there actually a way forward to resolution for this ticket, should it be left open, with a clearer message in the issue summary about what the purpose of this ticket is? Or should we commit the bug fixes (like #167 but maybe handle that in its own 'bug report' ticket) that have been found and then close this ticket out?

🇬🇧United Kingdom steven jones

We had it where a video had somehow ended up being classified as an image, so the formatter was fetching the image dimensions, not the video dimensions.

Given that getImageDimensions can return NULL in lots of scenarios, this module really should check for that!

Code in MR!203 seemed to work fine.

🇬🇧United Kingdom steven jones

We're also seeing this issue.

🇬🇧United Kingdom steven jones

We're seeing this issue too, in our case, it was a combination of using the shield module, and key module (with key overrides) triggering this issue for us.

Bizarrely enabling the dblog module sorted the issue, as did the patch from #81.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

Merged then, thanks for the review.

🇬🇧United Kingdom steven jones

This now needs a review, but this should be nice and compatible with what we had in the Drupal 7 version of the module.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

All done here.

🇬🇧United Kingdom steven jones

I've created a MR with a bit of code that makes sure that the error message element is in the viewport and visible using webform's scroll library.
I've also taken the liberty of showing how this code can be made common to both library files, and thus maybe a path forward to reducing the differences between the AJAX and non-AJAX versions of the code, when 95% of the code is the same between the two.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

At the moment I think this module is 'getting away with it' because Drupal core has a behavior in form.js that will prevent double submissions of a form. But there's no reason to not have the check in the MR I don't think?

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

See the MR for the code.

🇬🇧United Kingdom steven jones

steven jones created an issue.

🇬🇧United Kingdom steven jones

MR ready for review. It changes the global init flag for one specifically on the stripe payment element.

I'll admit that if you were to add two payment elements to the same webform this might cause more weird things to happen, but I think lots of weird things would be happening anyway in that case!

🇬🇧United Kingdom steven jones

Hah! Yeah, this is great. Removes the root just fine.

🇬🇧United Kingdom steven jones

No, I believe not. If there are further deprecations in the Drupal 11 lifecycle, then we'll want to know about those and those will get posted here I believe.

I might be wrong though. Will try to check.

🇬🇧United Kingdom steven jones

Oh, super interesting! Because I tested this, and it totally worked fine and the account was switched correctly when doing a batched export.
I was surprised by this tbh, and didn't think too much about it, though I can imagine that yeah, it doesn't work in all cases!

What version of Drush are you using? Maybe that's important somehow?

🇬🇧United Kingdom steven jones

I've raised the issue on purge module: 🐛 Purge module TagInvalidation allows spaces Active

🇬🇧United Kingdom steven jones

We're seeing this issue too. I feel like this could be handled upstream by the purger module, because if cache tags are never supposed to contain spaces, why is it passing them on to the purger plugins in the first place?

Anyway, we are where we are, and I might make a patch to put a band-aid on the issue by silently ignoring the tags with spaces, or splitting on space, and invalidating those tags, because that's presumably how they are being treated by some parts of the system.

🇬🇧United Kingdom steven jones

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

Production build 0.71.5 2024