Account created on 29 September 2011, over 13 years ago
  • Tech expert at Smile 
#

Merge Requests

More

Recent comments

đŸ‡«đŸ‡·France DrDam

@nikolay shapovalov : sorry for that, after nearly 2 month, I have presume it be ok for you ...

đŸ‡«đŸ‡·France DrDam

@nikolay shapovalov :
"Do you think we can add interface for these new methods?"

I don't know ...
The purpose of the RM is simply to separate the processing/generation of the derivatives from the various validation/authorisation operations linked to this generation.

@oily : I don't know either what @deviantintegral want to say, when it create the issue.
I just done my best when @smustgrave ask me to update summary to the "good template"
I have my use case "with mediaContextualCrop, we want to create a new image delivering methodebased on 80% of the ImageStyleDownloadController" for the others I don't know their motivations.

đŸ‡«đŸ‡·France DrDam

The "Null" label is created by the "->label()" method because at this stage (node creation) the "title" ( targeted by the method) is null. I.E. there is no "saved title" for the current entity.

đŸ‡«đŸ‡·France DrDam

fixed in 1.0.0@alpha2

đŸ‡«đŸ‡·France DrDam

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

đŸ‡«đŸ‡·France DrDam

Create MR for 11.x and reroll #16 on it

đŸ‡«đŸ‡·France DrDam

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

đŸ‡«đŸ‡·France DrDam

For test coverage, I don't what it need to be tested, it just code management, no new functionnality.

đŸ‡«đŸ‡·France DrDam

I have rework the "association loop" for trying to manage mixte between not-upscaling / upsaling.

The association between size and imageStyle are "not optimal" because of #10 and #11, but it doesn't really matter because the derivative will be the same, just not in the "best folder" on the disk

đŸ‡«đŸ‡·France DrDam

Exemple as a native src-set image :

The "tiniest" size ( 100px) better fit for the thumbnail image style are associate with the "wide" image style because it's the "last" of the list.

đŸ‡«đŸ‡·France DrDam

The problem is that the imageStyle stack does not provide a simple way of finding the ‘expected size of the derivative if the source image is large enough not to be upscaled’ for an image style.
In fact, it is not possible to ‘classify’ image styles by this ‘expected size’ and therefore to find the ‘smallest’ image style that is most suitable.

The anomaly is present in native ResponsiveImages too.

đŸ‡«đŸ‡·France DrDam

In fact, this case is not natively covered by nativ responsiveImage because of the possible mixe between "allow/no-allow" upscaling.

đŸ‡«đŸ‡·France DrDam

But if you editing the media, you change it on all its usage

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 3496234-better-handling-of to active.

đŸ‡«đŸ‡·France DrDam

@deepali sardana : thanks for the merge Request, but I'll close it, because there is a lighter way to acheive that.

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 2685905-refactor-imagestyledownloadcontroller-so to hidden.

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 3228376-cant-delete-a to hidden.

đŸ‡«đŸ‡·France DrDam

Can you make the MR for the 2.x branch ?

the 1.x branch are minimaly maintained now

thanks

đŸ‡«đŸ‡·France DrDam

diag : the point here is the same as many (many) other Crop/Image anomalies, Drupal Core don't know how handle multiple "version" of the same derivative ( I.E. couple ImageStyle x ImageFile ).

There is no real solution base of maintening the current implementation of the "focal point preview" feature.

2 solutions are possibles :
- Simulate the rendering (by CSS/html style manipulation) instead using the full derivative generation
- Add some "context" to the derivativ generation (I.E. hack some core process in order add this feature)

đŸ‡«đŸ‡·France DrDam

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

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 3214798-crop-effect-can to hidden.

đŸ‡«đŸ‡·France DrDam

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

đŸ‡«đŸ‡·France DrDam

You need to install the 2.3 and make the db update first, and after update to 2.4.

If it's the case, maybe a better "message/communication my be better"

đŸ‡«đŸ‡·France DrDam

When using media, you need some configurations :

  1. in the "host content type" (node)
    • in the form mode : media library as widget
    • in the view mode : display media with a specific render mode "foo"
  2. in the "media image"
    • in the form mode : the image widget crop widget
    • in the view mode "foo" : render image with a specific image style using a crop
đŸ‡«đŸ‡·France DrDam

Did you check about media_contextual_crop → ?

This module provide a way to wire crop and medias

đŸ‡«đŸ‡·France DrDam

I have split the doTestFileUriAlter method in order to have "clean" code separation

đŸ‡«đŸ‡·France DrDam

Merge in 3.0.2

đŸ‡«đŸ‡·France DrDam

Fix reviews & add test

đŸ‡«đŸ‡·France DrDam

I think this two issues (phpstan & phpcs) may be merge BEFORE other merge resquests

đŸ‡«đŸ‡·France DrDam

And at first sight it looks like methods: getCropFromImageStyle, getCropFromImageStyleId, cropExists, findCrop should be moved away from CropInterface. Maybe we need to make bigger refactor to impove developers expirience.

I totaly agree, but without knowing the history of the implementation, it's not clear how far the refactor can go.

đŸ‡«đŸ‡·France DrDam

The question is whether the current behavior is “intentional” or a case that was not anticipated.

đŸ‡«đŸ‡·France DrDam

Thanks @hartsak, I pass the ticket to RTBC

đŸ‡«đŸ‡·France DrDam

We are hosting on Pantheon which sits behind the Fastly CDN. Sounds like disabling flush_derivative_images is recommended with this setup?

that's what I would recommend. The "derivative cache" are supported by the CDN, so no need to "force drupal" to handle it.

I've read this a couple of times and I think that you are missing the word "no" in the above sentence, that "... there is no way to detect...". Can you clarify ?

You are right, I fix my message. There is no way

đŸ‡«đŸ‡·France DrDam

I have create a second merge-request for 2.x branch

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 3270150-can-media-library-2x to hidden.

đŸ‡«đŸ‡·France DrDam

drdam → changed the visibility of the branch 3270150- to hidden.

đŸ‡«đŸ‡·France DrDam

In Drupal 10.3+ , we have a new static method to do that

$converted_original_uri = ImageStyleDownloadController::getUriWithoutConvertedExtension($original_uri);

I'm open a merge request to add this

đŸ‡«đŸ‡·France DrDam

As a note, I think it's necessary to question the need to have 250 different image styles in the project, just for the issue of storage space consumption (green-it / eco-design / cost / greenwashing...).

Did you use sort of CDN or S3-like storage services ? In this case, the disabling of the flush_derivative_images, in settings are the way to fix the issue.

In crop entity perspective, there is way to detect if the newly created crop is created from a "new uploaded image" or because someone change the ImageStyle configuration to adding crop in it (and a derivative image already exist for the source and must be flushed).

đŸ‡«đŸ‡·France DrDam

You can check : https://www.drupal.org/project/crop/issues/2617818 ✹ Support multiple crop variants per URI and crop type Needs work

or the media_contextual_crop → collection

đŸ‡«đŸ‡·France DrDam

I agreed for breaking the loop for performance issues.
If the style have 5 effects and if the crop are the first, there are no use to check all others.

đŸ‡«đŸ‡·France DrDam

Seems like https://www.drupal.org/project/crop/issues/3293782 🐛 Crop API is not appending a hash when the image styles are converted to WEBP Needs review

đŸ‡«đŸ‡·France DrDam

Check https://www.drupal.org/project/crop/issues/3228376 🐛 Can't delete a crop type Needs review in order to purge and delete all crop entities

đŸ‡«đŸ‡·France DrDam

Some update here and a "redo" of the patch for Drupal 11

đŸ‡«đŸ‡·France DrDam

Waiting ImageWidgetCrop Drupal 11 release

Production build 0.71.5 2024