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

Closes #875 Add new option to select next-gen format #879

Merged
merged 17 commits into from
Apr 19, 2024

Conversation

Tabrisrp
Copy link
Contributor

Description

Add a new UI setting option to select the next-gen format to generate, or disable it.

Fixes #875

  • Enhancement (non-breaking change which improves an existing functionality).

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.

Observability

  • I handled errors when needed.
  •  I wrote user-facing messages that are understandable and provide actionable feedbacks.
  • I prepared ways to observe the implemented system (logs, data, etc.).

@Tabrisrp Tabrisrp added type: enhancement AVIF Avif branch/feature labels Apr 15, 2024
@Tabrisrp Tabrisrp added this to the 2.2.2 milestone Apr 15, 2024
@Tabrisrp Tabrisrp requested a review from a team April 15, 2024 20:43
@Tabrisrp Tabrisrp self-assigned this Apr 15, 2024
Copy link
Contributor

@Khadreal Khadreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

  • Won't it be nice to have off as a constant considering how often it's use and its subsequent use, and if the value change or need to be modify it can be done from one point.
    Also, for UI related PR, would it be an overkill to add a screenshot of it

@Mai-Saad
Copy link

Mai-Saad commented Apr 17, 2024

@Tabrisrp Thanks for the PR, please find exploratory test notes below (WIP)

  • Next-gen options shall match that on Epic i.e NONE then WEBP then AVIF => seems Notion wasn't updated and ok as is

Screenshot from 2024-04-17 19-25-59

  • Warning after uploading images with a fresh install of Imagify and optimization, it keeps loading
    1- fresh install and activate of Imagify
    2- upload image => see the warning
    Screenshot from 2024-04-18 09-07-47
    Screenshot from 2024-04-18 09-07-19

  • Fatal error when uploading image/ optimize image while filter is used
    1- fresh install/activation of Imagify
    2- Enable the Next-Gen filter

add_filter('imagify_nextgen_images_formats', function($formats){
    $formats['webp'] = 'webp';

    return $formats ;
});

3- upload an image or click optimize for a previously uploaded image

[18-Apr-2024 07:27:09 UTC] PHP Fatal error:  Uncaught TypeError: Imagify\Optimization\Process\AbstractProcess::get_suffix_from_format(): Argument #1 ($format) must be of type string, null given, called in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php on line 1554 and defined in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php:1570
Stack trace:
#0 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php(1554): Imagify\Optimization\Process\AbstractProcess->get_suffix_from_format()
#1 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php(500): Imagify\Optimization\Process\AbstractProcess->is_size_next_gen()
#2 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Job/MediaOptimization.php(181): Imagify\Optimization\Process\AbstractProcess->optimize_size()
#3 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Job/MediaOptimization.php(66): Imagify\Job\MediaOptimization->task_optimize()
#4 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/inc/classes/Dependencies/deliciousbrains/wp-background-processing/classes/wp-background-process.php(516): Imagify\Job\MediaOptimization->task()
#5 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/inc/classes/Dependencies/deliciousbrains/wp-background-processing/classes/wp-background-process.php(333): Imagify_WP_Background_Process->handle()
#6 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): Imagify_WP_Background_Process->maybe_handle()
#7 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#8 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#9 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-admin/admin-ajax.php(192): do_action()
#10 {main}
  thrown in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php on line 1570
[18-Apr-2024 08:20:03 UTC] PHP Deprecated:  strpos(): Passing null to parameter #2 ($needle) of type string is deprecated in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/WP.php on line 194
[18-Apr-2024 08:20:03 UTC] PHP Deprecated:  strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/WP.php on line 208
[18-Apr-2024 08:20:03 UTC] PHP Fatal error:  Uncaught Error: Undefined constant Imagify\Optimization\Process\WP::_SUFFIX in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/WP.php:208
Stack trace:
#0 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/WP.php(208): constant()
#1 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/Bulk.php(237): Imagify\Bulk\WP->get_optimized_media_ids_without_format()
#2 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/Bulk.php(606): Imagify\Bulk\Bulk->run_generate_nextgen()
#3 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(326): Imagify\Bulk\Bulk->maybe_generate_missing_nextgen()
#4 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#5 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#6 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/option.php(889): do_action()
#7 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-abstract-options.php(372): update_option()
#8 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-abstract-options.php(231): Imagify_Abstract_Options->set_raw()
#9 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/admin/upgrader.php(318): Imagify_Abstract_Options->set()
#10 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): _imagify_new_upgrade()
#11 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#12 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#13 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/admin/upgrader.php(69): do_action()
#14 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): _imagify_upgrader()
#15 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#16 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#17 /var/www/mai.rocketlabsqa.ovh/htdocs/wp-admin/admin-ajax.php(45): do_action()
#18 {main}
  thrown in /var/www/mai.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/Bulk/WP.php on line 208

Note: the same fatal error occurred if we enabled avif and save settings after a fresh install of 2.2.2

Copy link

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are good here, if UI modification with filter + read more link will be on other GH then we can merge this.
testrail-report-594.pdf

@Tabrisrp Tabrisrp added this pull request to the merge queue Apr 19, 2024
Merged via the queue into develop with commit 313dbb2 Apr 19, 2024
5 of 6 checks passed
@Tabrisrp Tabrisrp deleted the enhancement/875-new-option branch April 19, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AVIF Avif branch/feature type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to choose which next-gen images should be generated in UI
5 participants