Skip to content

Commit 4c2a88b

Browse files
MiraeldMiraeld
committed
Address review on the deferred optimization fix
Correct the docblock: WordPress has passed the context argument since 5.3, not 7.1. Note on the meta hooks why they are now registered for every version, and skip flagging an attachment Imagify cannot optimize. Register the new transient in InternalStateList so a reset and uninstall clear it, and derive the reset test's query count from that list. Add an integration test that fires the real hook chain rather than calling the methods directly: it proves nothing is optimized on the create phase, and that the pass which does run happens after the metadata is stored, so it reads the complete set of sub sizes.
1 parent 42db1ec commit 4c2a88b

6 files changed

Lines changed: 318 additions & 4 deletions

File tree

‎Tests/Integration/inc/classes/AutoOptimization/ClientSideUploadSequenceTest.php‎

Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
<?php
2+
3+
namespace Imagify\Tests\Integration\inc\classes\AutoOptimization;
4+
5+
use Imagify_Auto_Optimization;
6+
use Imagify\Tests\Integration\TestCase;
7+
use WP_REST_Request;
8+
9+
/**
10+
* Fires the real WordPress hook chain for a WP 7.1 client side upload and checks when auto
11+
* optimization runs, and what it can see when it does.
12+
*
13+
* The unit tests call the methods directly, which proves they behave as written but not that
14+
* the sequence WordPress actually produces lands where it should. This drives the genuine
15+
* chain instead: `wp_generate_attachment_metadata` for the create phase, then the same filter
16+
* for the finalize phase, then `wp_update_attachment_metadata`, which writes the post meta and
17+
* therefore fires `added_post_meta` / `updated_post_meta`.
18+
*
19+
* Two things matter and neither can be checked from a unit test:
20+
* - nothing is optimized on the create phase, when no sub size exists yet,
21+
* - the optimization that does run happens after the metadata is stored, so the sizes it
22+
* reads are the complete set rather than the value that was about to be replaced.
23+
*
24+
* @covers \Imagify_Auto_Optimization::maybe_store_generate_step
25+
* @covers \Imagify_Auto_Optimization::store_ids_to_optimize
26+
* @covers \Imagify_Auto_Optimization::do_auto_optimization_after_meta_update
27+
* @group AutoOptimization
28+
*/
29+
class Test_ClientSideUploadSequence extends TestCase {
30+
/**
31+
* This suite needs no Imagify API credentials.
32+
*
33+
* @var bool
34+
*/
35+
protected $useApi = false; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeCase
36+
37+
/**
38+
* Files written to disk by the test.
39+
*
40+
* @var array
41+
*/
42+
private $created_files = [];
43+
44+
/**
45+
* Auto optimization runs recorded during the test.
46+
*
47+
* @var array
48+
*/
49+
private $runs = [];
50+
51+
/**
52+
* Original value of the auto_optimize option.
53+
*
54+
* @var mixed
55+
*/
56+
private $original_auto_optimize;
57+
58+
/**
59+
* Prepares the test environment before each test.
60+
*/
61+
public function set_up() {
62+
parent::set_up();
63+
64+
$this->original_auto_optimize = get_imagify_option( 'auto_optimize' );
65+
update_imagify_option( 'auto_optimize', 1 );
66+
67+
$this->runs = [];
68+
69+
/*
70+
* Record every optimization the sequence triggers, along with the sizes readable from
71+
* the stored metadata at that moment. What is under test is when the decision is taken
72+
* and what it can see: the optimization itself only reaches a queue that nothing
73+
* dispatches during the tests.
74+
*/
75+
add_action(
76+
'imagify_before_auto_optimization',
77+
[ $this, 'record_run' ],
78+
5,
79+
2
80+
);
81+
82+
/*
83+
* The plugin registers these on boot. Calling init() again is idempotent, since the
84+
* callbacks and priorities are identical, and it keeps the test honest if the hooks
85+
* were not registered for any reason. remove_hooks() is deliberately not called on
86+
* tear down: it would leave auto optimization switched off for every later test.
87+
*/
88+
Imagify_Auto_Optimization::get_instance()->init();
89+
}
90+
91+
/**
92+
* Cleans up the test environment after each test.
93+
*/
94+
public function tear_down() {
95+
remove_action( 'imagify_before_auto_optimization', [ $this, 'record_run' ], 5 );
96+
97+
update_imagify_option( 'auto_optimize', $this->original_auto_optimize );
98+
99+
foreach ( $this->created_files as $file ) {
100+
if ( file_exists( $file ) ) {
101+
wp_delete_file( $file );
102+
}
103+
}
104+
105+
$this->created_files = [];
106+
107+
parent::tear_down();
108+
}
109+
110+
/**
111+
* Records an auto optimization run and what the stored metadata holds at that point.
112+
*
113+
* @param int $attachment_id Attachment ID.
114+
* @param bool $is_new_upload Whether Imagify treats this as a new upload.
115+
*/
116+
public function record_run( $attachment_id, $is_new_upload ) {
117+
$metadata = wp_get_attachment_metadata( $attachment_id );
118+
119+
$this->runs[] = [
120+
'is_new_upload' => $is_new_upload,
121+
'sizes' => is_array( $metadata ) && ! empty( $metadata['sizes'] ) ? array_keys( $metadata['sizes'] ) : [],
122+
];
123+
}
124+
125+
/**
126+
* Creates an attachment with a real file behind it, as the upload would.
127+
*
128+
* @return int
129+
*/
130+
private function create_attachment() {
131+
$uploads = wp_upload_dir();
132+
$filename = 'imagify-client-side-' . uniqid() . '.jpg';
133+
$file_path = trailingslashit( $uploads['basedir'] ) . $filename;
134+
135+
wp_mkdir_p( dirname( $file_path ) );
136+
file_put_contents( $file_path, 'not-a-real-jpeg' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents
137+
138+
$this->created_files[] = $file_path;
139+
140+
$attachment_id = $this->factory()->attachment->create_object(
141+
[
142+
'file' => $filename,
143+
'post_mime_type' => 'image/jpeg',
144+
'post_status' => 'inherit',
145+
]
146+
);
147+
148+
update_post_meta( $attachment_id, '_wp_attached_file', $filename );
149+
150+
return $attachment_id;
151+
}
152+
153+
/**
154+
* Builds the REST request WordPress hands to `rest_after_insert_attachment` when the
155+
* browser is going to send the sub sizes itself.
156+
*
157+
* @return WP_REST_Request
158+
*/
159+
private function client_side_request() {
160+
$request = new WP_REST_Request( 'POST', '/wp/v2/media' );
161+
$request->set_param( 'generate_sub_sizes', false );
162+
163+
return $request;
164+
}
165+
166+
/**
167+
* Runs the create phase: WordPress stores metadata that carries no sub size yet.
168+
*
169+
* @param int $attachment_id Attachment ID.
170+
*/
171+
private function run_create_phase( $attachment_id ) {
172+
$metadata = [
173+
'file' => get_post_meta( $attachment_id, '_wp_attached_file', true ),
174+
'width' => 3800,
175+
'height' => 2500,
176+
'sizes' => [],
177+
];
178+
179+
/** This filter is documented in wp-admin/includes/image.php */
180+
$metadata = apply_filters( 'wp_generate_attachment_metadata', $metadata, $attachment_id, 'create' );
181+
182+
wp_update_attachment_metadata( $attachment_id, $metadata );
183+
}
184+
185+
/**
186+
* Runs the finalize phase: every sideloaded sub size is stored in one go.
187+
*
188+
* @param int $attachment_id Attachment ID.
189+
*/
190+
private function run_finalize_phase( $attachment_id ) {
191+
$metadata = wp_get_attachment_metadata( $attachment_id );
192+
193+
$metadata['sizes'] = [
194+
'thumbnail' => [ 'file' => 'thumb.jpg' ],
195+
'medium' => [ 'file' => 'medium.jpg' ],
196+
'medium_large' => [ 'file' => 'medium_large.jpg' ],
197+
'large' => [ 'file' => 'large.jpg' ],
198+
];
199+
200+
/** This filter is documented in wp-admin/includes/image.php */
201+
$metadata = apply_filters( 'wp_generate_attachment_metadata', $metadata, $attachment_id, 'update' );
202+
203+
wp_update_attachment_metadata( $attachment_id, $metadata );
204+
}
205+
206+
/**
207+
* Test: the create phase optimizes nothing, and the finalize phase optimizes once, as a new
208+
* upload, with every sub size readable from the stored metadata.
209+
*/
210+
public function testOptimizesOnceTheSubsizesAreStored() {
211+
$attachment_id = $this->create_attachment();
212+
213+
Imagify_Auto_Optimization::get_instance()->flag_awaiting_client_side_subsizes(
214+
get_post( $attachment_id ),
215+
$this->client_side_request(),
216+
true
217+
);
218+
219+
$this->run_create_phase( $attachment_id );
220+
221+
$this->assertSame( [], $this->runs, 'Nothing should be optimized while the browser still owes its sub sizes.' );
222+
223+
$this->run_finalize_phase( $attachment_id );
224+
225+
$this->assertCount( 1, $this->runs, 'The media should be optimized exactly once.' );
226+
$this->assertTrue( $this->runs[0]['is_new_upload'], 'The finalize pass is still the new upload.' );
227+
$this->assertSame(
228+
[ 'thumbnail', 'medium', 'medium_large', 'large' ],
229+
$this->runs[0]['sizes'],
230+
'The optimization must run after the metadata is stored, so every sub size is visible.'
231+
);
232+
}
233+
234+
/**
235+
* Test: an ordinary upload, where WordPress builds the sub sizes itself, is optimized on
236+
* the create phase exactly as before, so the deferral is limited to the client side flow.
237+
*/
238+
public function testOptimizesImmediatelyOnAnOrdinaryUpload() {
239+
$attachment_id = $this->create_attachment();
240+
241+
// No flag: WordPress is building the sub sizes itself.
242+
$metadata = [
243+
'file' => get_post_meta( $attachment_id, '_wp_attached_file', true ),
244+
'width' => 1200,
245+
'height' => 900,
246+
'sizes' => [
247+
'thumbnail' => [ 'file' => 'thumb.jpg' ],
248+
'medium' => [ 'file' => 'medium.jpg' ],
249+
],
250+
];
251+
252+
/** This filter is documented in wp-admin/includes/image.php */
253+
$metadata = apply_filters( 'wp_generate_attachment_metadata', $metadata, $attachment_id, 'create' );
254+
255+
wp_update_attachment_metadata( $attachment_id, $metadata );
256+
257+
$this->assertCount( 1, $this->runs, 'An ordinary upload should still be optimized once, on the create phase.' );
258+
$this->assertTrue( $this->runs[0]['is_new_upload'] );
259+
}
260+
261+
/**
262+
* Test: the flag is not left behind once the sub sizes have arrived.
263+
*/
264+
public function testClearsTheFlagOnceTheSubsizesArrive() {
265+
$attachment_id = $this->create_attachment();
266+
$auto = Imagify_Auto_Optimization::get_instance();
267+
268+
$auto->flag_awaiting_client_side_subsizes( get_post( $attachment_id ), $this->client_side_request(), true );
269+
270+
$this->assertTrue( $auto->is_awaiting_client_side_subsizes( $attachment_id ) );
271+
272+
$this->run_create_phase( $attachment_id );
273+
274+
$this->assertTrue( $auto->is_awaiting_client_side_subsizes( $attachment_id ), 'The flag survives the create phase.' );
275+
276+
$this->run_finalize_phase( $attachment_id );
277+
278+
$this->assertFalse( $auto->is_awaiting_client_side_subsizes( $attachment_id ), 'The flag is cleared once the sub sizes are in.' );
279+
}
280+
}

‎Tests/Unit/classes/Tools/InternalStateList/sharedList.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ public function testGetLockedTransientPatternsReturnsExpectedArray(): void {
6868
'_transient_%imagify_rpc_%',
6969
'_transient_imagify_%_process_locked',
7070
'_site_transient_imagify_%_process_lock%',
71+
'_transient_imagify_awaiting_subsizes_%',
72+
'_transient_timeout_imagify_awaiting_subsizes_%',
7173
];
7274

7375
$this->assertSame( $expected, InternalStateList::get_locked_transient_patterns() );

‎Tests/Unit/classes/Tools/ResetInternalState/reset.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
namespace Imagify\Tests\Unit\classes\Tools\ResetInternalState;
55

66
use Imagify\Tests\Unit\TestCase;
7+
use Imagify\Tools\InternalStateList;
78
use Imagify\Tools\ResetInternalState;
89
use Mockery;
910
use Brain\Monkey\Functions;
@@ -161,7 +162,7 @@ function ( string $sql, string $pattern ) use ( &$patterns_queried ) {
161162
}
162163
);
163164

164-
$this->wpdb->shouldReceive( 'query' )->times( 4 )->andReturn( 0 );
165+
$this->wpdb->shouldReceive( 'query' )->times( count( InternalStateList::get_locked_transient_patterns() ) )->andReturn( 0 );
165166

166167
( new ResetInternalState() )->reset();
167168

@@ -171,6 +172,8 @@ function ( string $sql, string $pattern ) use ( &$patterns_queried ) {
171172
'\_transient\_%imagify\_rpc\_%',
172173
'\_transient\_imagify\_%\_process\_locked',
173174
'\_site\_transient\_imagify\_%\_process\_lock%',
175+
'\_transient\_imagify\_awaiting\_subsizes\_%',
176+
'\_transient\_timeout\_imagify\_awaiting\_subsizes\_%',
174177
];
175178

176179
foreach ( $expected_patterns as $pattern ) {
@@ -263,7 +266,7 @@ function () use ( &$query_calls ) {
263266

264267
( new ResetInternalState() )->reset();
265268

266-
// 4 options-pattern queries prove reset() ran to completion.
267-
$this->assertSame( 4, $query_calls );
269+
// One options-pattern query per registered pattern proves reset() ran to completion.
270+
$this->assertSame( count( InternalStateList::get_locked_transient_patterns() ), $query_calls );
268271
}
269272
}

‎Tests/Unit/inc/classes/AutoOptimization/MaybeStoreGenerateStepTest.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ private function requestReturning( $value ) {
105105
* Test: the attachment is flagged when WordPress hands the sub sizes over to the browser.
106106
*/
107107
public function testFlagsTheAttachmentWhenTheBrowserHandlesTheSubsizes(): void {
108+
$this->stubDependencies();
109+
108110
$stored = [];
109111

110112
Functions\when( 'set_transient' )->alias(
@@ -124,6 +126,7 @@ function ( $name, $value ) use ( &$stored ) {
124126
* nor when the parameter is absent entirely.
125127
*/
126128
public function testDoesNotFlagWhenWordPressBuildsTheSubsizes(): void {
129+
$this->stubDependencies();
127130
Functions\expect( 'set_transient' )->never();
128131

129132
$attachment = (object) [ 'ID' => 42 ];
@@ -132,10 +135,21 @@ public function testDoesNotFlagWhenWordPressBuildsTheSubsizes(): void {
132135
( new Imagify_Auto_Optimization() )->flag_awaiting_client_side_subsizes( $attachment, $this->requestReturning( null ), true );
133136
}
134137

138+
/**
139+
* Test: an attachment Imagify cannot optimize is not flagged, since nothing would read it.
140+
*/
141+
public function testDoesNotFlagAnUnsupportedMimeType(): void {
142+
Functions\when( 'imagify_is_attachment_mime_type_supported' )->justReturn( false );
143+
Functions\expect( 'set_transient' )->never();
144+
145+
( new Imagify_Auto_Optimization() )->flag_awaiting_client_side_subsizes( (object) [ 'ID' => 42 ], $this->requestReturning( false ), true );
146+
}
147+
135148
/**
136149
* Test: nothing is flagged when an existing attachment is being updated rather than created.
137150
*/
138151
public function testDoesNotFlagWhenUpdatingAnAttachment(): void {
152+
$this->stubDependencies();
139153
Functions\expect( 'set_transient' )->never();
140154

141155
( new Imagify_Auto_Optimization() )->flag_awaiting_client_side_subsizes( (object) [ 'ID' => 42 ], $this->requestReturning( false ), false );

‎classes/Tools/InternalStateList.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ public static function get_locked_transient_patterns(): array {
6464
'_transient_%imagify_rpc_%', // Legacy/deprecated.
6565
'_transient_imagify_%_process_locked',
6666
'_site_transient_imagify_%_process_lock%',
67+
// Flags an attachment whose sub sizes the browser is still to send, on WP 7.1+.
68+
'_transient_imagify_awaiting_subsizes_%',
69+
'_transient_timeout_imagify_awaiting_subsizes_%',
6770
];
6871
}
6972

0 commit comments

Comments
 (0)