Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about PluginCheck.CodeAnalysis.ImageFunctions.NonEnqueuedImage sniff #831

Open
jcvignoli opened this issue Dec 13, 2024 · 5 comments
Labels
[Team] Performance Issues owned by Performance Team

Comments

@jcvignoli
Copy link

jcvignoli commented Dec 13, 2024

Hi there,

I don't understand the point of this new PluginCheck.CodeAnalysis.ImageFunctions.NonEnqueuedImage as implemented there:

plugin-check/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/ImageFunctionsSniff.php

which results in Images should be added using wp_get_attachment_image() or similar functions

For many of the WP plugins makers , we add images in the WP admin backoffice. Those images are not uploaded to WP gallery, but rather static files called the good old way with an . There is no way to bypass the regex, which will detect any in the plugin.

It is my understanding you're going to change it a warning, as discussed here #757 but I'm questionning even such a warning. To my understanding, even a warning is not appropriate for plugins makers.

Even if the aim is undertandable, in practice it doesn't make sense to me.

@ernilambar ernilambar added the [Team] Performance Issues owned by Performance Team label Dec 13, 2024
@swissspidy
Copy link
Member

A static code analysis tool does not know whether an image is used in the WP admin or elsewhere. So the idea behind that was to warn about this to catch cases where another function may be more appropriate. In some places that's not possible, in which case it can be ignored.

What we might be able to do here is enhancing this check to look for things like width, height and only emit a warning if these are missing. Maybe for srcset and sizes too. Curious to hear other thoughts from the performance folks.

@jcvignoli
Copy link
Author

A static code analysis tool does not know whether an image is used in the WP admin or elsewhere.

I understand. But this may be show this rule has been implemented too soon, precisely.

Please be aware that the rule throws errors, not warnings. My CI fails, I disabled a month ago for that very reason.

@swissspidy
Copy link
Member

As of #822 it's only a warning, it just hasn't been published in a new release yet.

@jcvignoli
Copy link
Author

I don't get it.

The github action is still throwing errors, so is the wordpress plugin .

Using github action "main", and latest wordpress plugin. Both throw errors.

@yalogica
Copy link

I have the same issues with build-in images in some of my plugins. The Plugin Check show errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Team] Performance Issues owned by Performance Team
Projects
None yet
Development

No branches or pull requests

4 participants