From ba2ba74540f86ba95d9f7be590260c487f44992c Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 14 Aug 2026 15:29:58 +0100 Subject: [PATCH 1/7] Derive the generated stylesheet's cache-busting version from its content FrmStylesController::get_css_version() reads frm_last_style_update and appends it as the ?ver= on the enqueued generated stylesheet. FrmStyle::save_settings() set that option to gmdate( 'njGi' ) - unpadded month, day and hour with no year - so the value was not a reliable marker of the file's content: * Distinct dates collide. 1 Jan 10:59, 11 Jan 00:59 and 1 Nov 00:59 all produce "111059". Sampling a year at three minutes per hour, 1,980 version strings are produced by more than one date, covering 17.4% of the timestamps tested. * With no year, every value repeats annually. * At minute resolution, two saves within the same minute are indistinguishable. A third-party CSS cache keyed on the enqueued URL can therefore keep serving a copy generated from superseded content. Use substr( md5( $css ), 0, 12 ) so the version changes if and only if the generated bytes change. Because the value is content-derived it is idempotent, so it must only be published once the bytes are known to have reached disk. FrmCreateFile::create_file() returned nothing and no-ops silently when it lacks filesystem permission or cannot create its directories; it now returns bool. The version is stored only on a confirmed write and after frmpro_css is populated, so it can never advertise content that is not being served. Publishing a hash for a failed write would pin a downstream cache to the superseded file permanently, since every later save of the same content reproduces the same hash and the same URL. migrate_to_107() discards the legacy value so the enqueued URL changes on upgrade even when the post-upgrade style regeneration is skipped by its function_exists( 'get_filesystem_method' ) guard. Co-Authored-By: Claude Opus 5 --- classes/helpers/FrmAppHelper.php | 2 +- classes/models/FrmCreateFile.php | 10 ++++---- classes/models/FrmMigrate.php | 22 ++++++++++++++++- classes/models/FrmStyle.php | 41 ++++++++++++++++++++++++++++---- 4 files changed, 65 insertions(+), 10 deletions(-) diff --git a/classes/helpers/FrmAppHelper.php b/classes/helpers/FrmAppHelper.php index 7d1532ea75..22f54185c0 100644 --- a/classes/helpers/FrmAppHelper.php +++ b/classes/helpers/FrmAppHelper.php @@ -10,7 +10,7 @@ class FrmAppHelper { * * @var int */ - public static $db_version = 106; + public static $db_version = 107; /** * Used by the API add-on. diff --git a/classes/models/FrmCreateFile.php b/classes/models/FrmCreateFile.php index 0b41d4fa64..9a30a678a2 100644 --- a/classes/models/FrmCreateFile.php +++ b/classes/models/FrmCreateFile.php @@ -76,13 +76,15 @@ private function set_new_file_path( $atts ) { } /** + * @since x.x Returns whether the file was written. Previously returned nothing. + * * @param string $file_content * - * @return void + * @return bool True if the file was written to disk. */ public function create_file( $file_content ) { if ( ! $this->has_permission ) { - return; + return false; } $dirs_exist = true; @@ -92,11 +94,11 @@ public function create_file( $file_content ) { // Only write the file if the folders exist. if ( ! $dirs_exist ) { - return; + return false; } global $wp_filesystem; - $wp_filesystem->put_contents( $this->new_file_path, $file_content, $this->chmod_file ); + return (bool) $wp_filesystem->put_contents( $this->new_file_path, $file_content, $this->chmod_file ); } /** diff --git a/classes/models/FrmMigrate.php b/classes/models/FrmMigrate.php index cee115c6da..47df93b54f 100644 --- a/classes/models/FrmMigrate.php +++ b/classes/models/FrmMigrate.php @@ -405,7 +405,7 @@ private function migrate_data( $old_db_version ) { return; } - $migrations = array( 16, 11, 16, 17, 23, 25, 86, 90, 97, 98, 101, 104, 105 ); + $migrations = array( 16, 11, 16, 17, 23, 25, 86, 90, 97, 98, 101, 104, 105, 107 ); foreach ( $migrations as $migration ) { if ( FrmAppHelper::$db_version < $migration || $old_db_version >= $migration ) { @@ -484,6 +484,26 @@ public function uninstall() { return true; } + /** + * Discard the legacy generated-stylesheet cache-busting version. + * + * Versions before this stored frm_last_style_update as gmdate( 'njGi' ), which could repeat + * across different dates and every year, so a third-party CSS cache keyed on the enqueued + * stylesheet URL could keep serving a copy generated from superseded content. + * + * Removing the stored value makes FrmStylesController::get_css_version() fall back to the + * plugin version, which guarantees the enqueued URL changes on this upgrade even when the + * post-upgrade style regeneration is skipped. The content-derived value is written by the + * next FrmStyle::save_settings() call. + * + * @since x.x + * + * @return void + */ + private function migrate_to_107() { + delete_option( 'frm_last_style_update' ); + } + /** * In older versions of Lite, it's possible we've saved the wrong location ID. * So force it to get valid values again. diff --git a/classes/models/FrmStyle.php b/classes/models/FrmStyle.php index 7cb8e3bb13..60bdea2ce4 100644 --- a/classes/models/FrmStyle.php +++ b/classes/models/FrmStyle.php @@ -407,7 +407,6 @@ public function get_color_settings() { */ public function save_settings() { $filename = FrmAppHelper::plugin_path() . '/css/custom_theme.css.php'; - update_option( 'frm_last_style_update', gmdate( 'njGi' ) ); if ( ! is_file( $filename ) ) { return; @@ -415,12 +414,46 @@ public function save_settings() { $this->clear_cache(); - $css = $this->get_css_content( $filename ); - $create_file = new FrmCreateFile( self::get_create_style_file_args() ); - $create_file->create_file( $css ); + $css = $this->get_css_content( $filename ); + $create_file = new FrmCreateFile( self::get_create_style_file_args() ); + $file_written = $create_file->create_file( $css ); update_option( 'frmpro_css', $css, false ); set_transient( 'frmpro_css', $css, MONTH_IN_SECONDS ); + + if ( $file_written ) { + self::update_css_version( $css ); + } + } + + /** + * Store the cache-busting version for the generated stylesheet. + * + * The value is derived from the stylesheet contents rather than the clock, so it changes if + * and only if the generated bytes change. It is read back by + * FrmStylesController::get_css_version() and appended to the enqueued stylesheet URL. + * + * This previously used gmdate( 'njGi' ), which omitted the year and concatenated unpadded + * month, day and hour values. Distinct dates therefore produced identical version strings + * (1 Jan 10:59, 11 Jan 00:59 and 1 Nov 00:59 all produced "111059"), the value repeated + * every year, and two saves within the same minute were indistinguishable. Third-party CSS + * caches keyed on the enqueued URL could keep serving a copy generated from superseded + * content. + * + * Only called once FrmCreateFile::create_file() confirms the bytes reached disk, and only + * after the frmpro_css option and transient are stored, so the version never advertises + * content that is not being served. This matters because the value is content-derived and + * therefore idempotent: publishing a hash for a write that silently failed would pin a + * third-party cache to the superseded file permanently, since every later save of the same + * content would reproduce the same hash and the same URL. + * + * @since x.x + * + * @param string $css Generated stylesheet contents. + * @return void + */ + private static function update_css_version( $css ) { + update_option( 'frm_last_style_update', substr( md5( $css ), 0, 12 ) ); } /** From 303d693e33cf1d88e6b3c95303b16b67a5fcfa2f Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 14 Aug 2026 15:30:19 +0100 Subject: [PATCH 2/7] Add tests for the content-derived stylesheet version The failure mode here is a version that looks fresh while the bytes behind it are not, so each test is written to fail against the previous implementation rather than merely exercise the new one: * The collision test documents that gmdate( 'njGi' ) returns "111059" for all three of 1 Jan 10:59, 11 Jan 00:59 and 1 Nov 00:59, then shows the new derivation is clock-independent - distinct content gives distinct versions at those same moments, identical content gives identical ones. * Content sensitivity is asserted in both directions. The second direction is why a hash was chosen over time(): re-saving an unchanged style must not churn visitor caches. * Two saves within one minute with different content produce different versions. * An integration test reads the registered 'formidable' handle's version from $wp_styles, changes a colour, saves again and asserts the version moved. * The migration is exercised through migrate_data()'s dispatch rather than called directly, so a migration that never runs cannot pass. It goes through migrate_data() rather than upgrade() because upgrade() regenerates the default style on every call, which would repopulate the option and mask the result. * A guard asserts gmdate( 'njGi' ) is not reintroduced into save_settings(). * The write-failure guard forces a real filesystem failure - uploads/formidable is replaced with a plain file so the required subdirectory cannot be created - and asserts the stored version is left untouched. * create_file()'s new bool contract is covered on all three paths, and append_file()/combine_files() are asserted unaffected by the widened return. Co-Authored-By: Claude Opus 5 --- tests/phpunit/database/test_FrmMigrate.php | 38 ++++ tests/phpunit/misc/test_FrmCreateFile.php | 158 +++++++++++++ tests/phpunit/styles/test_FrmStyle.php | 207 ++++++++++++++++++ .../styles/test_FrmStylesController.php | 62 ++++++ 4 files changed, 465 insertions(+) create mode 100644 tests/phpunit/misc/test_FrmCreateFile.php diff --git a/tests/phpunit/database/test_FrmMigrate.php b/tests/phpunit/database/test_FrmMigrate.php index 8721970356..d41922dced 100644 --- a/tests/phpunit/database/test_FrmMigrate.php +++ b/tests/phpunit/database/test_FrmMigrate.php @@ -297,6 +297,44 @@ public function test_migrate_to_97() { } } + /** + * Make sure migrate_to_107 is actually reached by the migration dispatch (migrate_data()) + * when upgrading from db_version 106, not just that the method works when called directly. + * A migration that is registered but never dispatched is the failure mode this guards + * against. + * + * This deliberately calls migrate_data() directly rather than the public upgrade(), because + * upgrade() unconditionally regenerates the default style at the end of every call + * ( $frm_style->update( 'default' ) ), which would overwrite frm_last_style_update with a + * fresh value regardless of whether migrate_to_107 actually ran, masking the result. + * + * @covers FrmMigrate::migrate_data + * @covers FrmMigrate::migrate_to_107 + */ + public function test_migrate_to_107_runs_on_dispatch_from_106() { + update_option( 'frm_last_style_update', '111059' ); + + $frmdb = new FrmMigrate(); + $this->run_private_method( array( $frmdb, 'migrate_data' ), array( 106 ) ); + + $this->assertFalse( get_option( 'frm_last_style_update' ), 'migrate_to_107 should have been dispatched and deleted the legacy frm_last_style_update option.' ); + } + + /** + * A site already on db_version 107 (or later) must not have migrate_to_107 run again. + * + * @covers FrmMigrate::migrate_data + * @covers FrmMigrate::migrate_to_107 + */ + public function test_migrate_to_107_does_not_run_again_once_applied() { + update_option( 'frm_last_style_update', 'abcdef123456' ); + + $frmdb = new FrmMigrate(); + $this->run_private_method( array( $frmdb, 'migrate_data' ), array( 107 ) ); + + $this->assertSame( 'abcdef123456', get_option( 'frm_last_style_update' ), 'migrate_to_107 must not re-run once already at db_version 107.' ); + } + /** * @covers FrmMigrate::collation */ diff --git a/tests/phpunit/misc/test_FrmCreateFile.php b/tests/phpunit/misc/test_FrmCreateFile.php new file mode 100644 index 0000000000..c523ad7364 --- /dev/null +++ b/tests/phpunit/misc/test_FrmCreateFile.php @@ -0,0 +1,158 @@ + + */ + private $paths_to_clean_up = array(); + + public function tearDown(): void { + foreach ( $this->paths_to_clean_up as $path ) { + if ( is_dir( $path ) ) { + @rmdir( $path ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + } elseif ( file_exists( $path ) ) { + @unlink( $path ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + } + } + $this->paths_to_clean_up = array(); + + parent::tearDown(); + } + + /** + * @covers FrmCreateFile::create_file + */ + public function test_create_file_returns_true_on_successful_write() { + $uploads = wp_upload_dir(); + $folder = 'frm-test-create-file-' . wp_generate_password( 8, false ); + $file_name = 'test.css'; + + $create_file = new FrmCreateFile( + array( + 'file_name' => $file_name, + 'folder_name' => $folder, + ) + ); + + $content = 'body{color:#123456}'; + $result = $create_file->create_file( $content ); + + $written_path = $uploads['basedir'] . '/' . $folder . '/' . $file_name; + $this->paths_to_clean_up[] = $written_path; + $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder . '/index.php'; + $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder; + + $this->assertTrue( $result, 'create_file() should return true when the file is actually written to disk.' ); + $this->assertSame( $content, file_get_contents( $written_path ) ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + } + + /** + * The write-permission early return (`! $this->has_permission`) can only be reached in this + * harness by forcing the private flag directly: the local/direct filesystem method always + * succeeds here, and there is no practical, non-flaky way to make WP_Filesystem's credential + * check fail deterministically in an automated integration run. Reflection on the private + * property is used only for this one guard clause, not for the method under test itself. + * + * @covers FrmCreateFile::create_file + */ + public function test_create_file_returns_false_without_permission() { + $create_file = new FrmCreateFile( + array( + 'file_name' => 'no-permission.css', + 'folder_name' => 'frm-test-no-permission', + ) + ); + + $permission_property = $this->get_accessible_property( $create_file, 'has_permission' ); + $permission_property->setValue( $create_file, false ); + + $result = $create_file->create_file( 'body{color:#000}' ); + + $this->assertFalse( $result, 'create_file() should return false when there is no filesystem permission.' ); + } + + /** + * Forces the directory-creation early return (`! $dirs_exist`) with a real filesystem + * collision rather than a mock: a plain file is created where FrmCreateFile needs to create a + * directory of the same name, so both mkdir() and the is_dir() fallback genuinely fail. + * + * @covers FrmCreateFile::create_file + */ + public function test_create_file_returns_false_when_directory_cannot_be_created() { + $uploads = wp_upload_dir(); + $blocking = 'frm-test-blocking-' . wp_generate_password( 8, false ); + $blocked_path = $uploads['basedir'] . '/' . $blocking; + + $this->assertNotFalse( file_put_contents( $blocked_path, 'not a directory' ), 'Test setup: failed to create the blocking file.' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + $this->paths_to_clean_up[] = $blocked_path; + + $create_file = new FrmCreateFile( + array( + 'file_name' => 'blocked.css', + 'folder_name' => $blocking, + ) + ); + + $result = $create_file->create_file( 'body{color:#000}' ); + + $this->assertFalse( $result, 'create_file() should return false when its target directory cannot be created.' ); + } + + /** + * The widened create_file() return type (void -> bool) must not affect its two internal + * callers: both discard the return value and remain void themselves. + * + * @covers FrmCreateFile::append_file + * @covers FrmCreateFile::combine_files + */ + public function test_append_file_and_combine_files_are_unaffected_by_the_widened_return_type() { + $uploads = wp_upload_dir(); + $folder = 'frm-test-callers-' . wp_generate_password( 8, false ); + + $append_target = new FrmCreateFile( + array( + 'file_name' => 'append.css', + 'folder_name' => $folder, + ) + ); + + $append_result = $append_target->append_file( 'first-part;' ); + $this->assertNull( $append_result, 'append_file() must remain void regardless of create_file()\'s widened return type.' ); + + $append_result_2 = $append_target->append_file( 'second-part;' ); + $this->assertNull( $append_result_2 ); + + $appended_path = $uploads['basedir'] . '/' . $folder . '/append.css'; + $this->paths_to_clean_up[] = $appended_path; + $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder . '/index.php'; + $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder; + + $this->assertSame( 'first-part;second-part;', file_get_contents( $appended_path ) ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + + $source_a = $uploads['basedir'] . '/' . $folder . '/source-a.css'; + $source_b = $uploads['basedir'] . '/' . $folder . '/source-b.css'; + file_put_contents( $source_a, 'a{color:#111}' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + file_put_contents( $source_b, 'b{color:#222}' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + $this->paths_to_clean_up[] = $source_a; + $this->paths_to_clean_up[] = $source_b; + + $combine_target = new FrmCreateFile( + array( + 'file_name' => 'combined.css', + 'folder_name' => $folder, + ) + ); + + $combine_result = $combine_target->combine_files( array( $source_a, $source_b ) ); + $this->assertNull( $combine_result, 'combine_files() must remain void regardless of create_file()\'s widened return type.' ); + + $combined_path = $uploads['basedir'] . '/' . $folder . '/combined.css'; + $this->paths_to_clean_up[] = $combined_path; + + $this->assertSame( "a{color:#111}\nb{color:#222}\n", file_get_contents( $combined_path ) ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + } +} diff --git a/tests/phpunit/styles/test_FrmStyle.php b/tests/phpunit/styles/test_FrmStyle.php index 5c8bb689b8..8c638d014b 100644 --- a/tests/phpunit/styles/test_FrmStyle.php +++ b/tests/phpunit/styles/test_FrmStyle.php @@ -156,4 +156,211 @@ private function trim_braces( $value ) { $frm_style = new FrmStyle(); return $this->run_private_method( array( $frm_style, 'trim_braces' ), array( $value ) ); } + + /** + * Regression test for the bug this fix addresses: the legacy `gmdate( 'njGi' )` cache-busting + * version omitted the year and concatenated unpadded month/day/hour, so distinct dates could + * produce an identical version string. This is documented directly (no clock mocking needed, + * since gmdate() accepts an explicit timestamp), then the new content-derived version is shown + * to both distinguish and, when content really is unchanged, correctly match across those same + * colliding moments -- proving the new value depends on content, not the clock. + * + * @covers FrmStyle::update_css_version + */ + public function test_update_css_version_does_not_collide_across_previously_colliding_dates() { + // 2026-01-01 10:59 UTC, 2026-01-11 00:59 UTC and 2026-11-01 00:59 UTC. + $colliding_timestamps = array( + gmmktime( 10, 59, 0, 1, 1, 2026 ), + gmmktime( 0, 59, 0, 1, 11, 2026 ), + gmmktime( 0, 59, 0, 11, 1, 2026 ), + ); + + $legacy_versions = array(); + foreach ( $colliding_timestamps as $timestamp ) { + $legacy_versions[] = gmdate( 'njGi', $timestamp ); + } + + // Document the bug being fixed: the legacy format collides across all three dates. + $this->assertSame( + array( '111059', '111059', '111059' ), + $legacy_versions, + 'Sanity check that these three dates are the ones known to collide under the legacy gmdate( "njGi" ) format.' + ); + + delete_option( 'frm_last_style_update' ); + + // Distinct content "saved" at each of those colliding moments must produce distinct + // versions. This is the property the legacy format could not provide. + $distinct_versions = array(); + foreach ( $colliding_timestamps as $index => $timestamp ) { + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{--collision-check:' . $index . '}' ) ); + $distinct_versions[] = get_option( 'frm_last_style_update' ); + } + + $this->assertCount( 3, array_unique( $distinct_versions ), 'The content-derived version must not collide across the three previously-colliding dates.' ); + + // Identical content "saved" at each of those same moments must produce the SAME version, + // proving the derivation is clock-independent rather than merely higher resolution. + $stable_versions = array(); + foreach ( $colliding_timestamps as $timestamp ) { + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#123456}' ) ); + $stable_versions[] = get_option( 'frm_last_style_update' ); + } + + $this->assertCount( 1, array_unique( $stable_versions ), 'Identical content must resolve to the same version regardless of when it is saved.' ); + } + + /** + * @covers FrmStyle::update_css_version + */ + public function test_update_css_version_is_sensitive_to_content_in_both_directions() { + delete_option( 'frm_last_style_update' ); + + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#111111}' ) ); + $version_a = get_option( 'frm_last_style_update' ); + $this->assertNotEmpty( $version_a ); + + // Different CSS must produce a different version. + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#222222}' ) ); + $version_b = get_option( 'frm_last_style_update' ); + $this->assertNotSame( $version_a, $version_b, 'Different CSS content must produce a different version.' ); + + // Identical CSS must produce the identical version (this is why a hash was chosen over + // time() -- an unchanged save should not needlessly invalidate downstream caches). + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#111111}' ) ); + $version_a_again = get_option( 'frm_last_style_update' ); + $this->assertSame( $version_a, $version_a_again, 'Identical CSS content must produce the identical version.' ); + } + + /** + * Two saves within the same minute (in fact, within the same PHP process) with different + * content must produce different versions. This fails by construction against the legacy + * gmdate( 'njGi' ) implementation, which is minute-granularity and would store the identical + * value for both saves. + * + * @covers FrmStyle::update_css_version + */ + public function test_same_minute_saves_with_different_content_produce_different_versions() { + delete_option( 'frm_last_style_update' ); + + $before = time(); + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#aaaaaa}' ) ); + $version_1 = get_option( 'frm_last_style_update' ); + + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#bbbbbb}' ) ); + $version_2 = get_option( 'frm_last_style_update' ); + $after = time(); + + // Sanity check that this test actually exercises the same-minute case it claims to. + $this->assertSame( gmdate( 'njGi', $before ), gmdate( 'njGi', $after ), 'Test setup assumption: both saves happened within the same legacy gmdate( "njGi" ) bucket.' ); + + $this->assertNotEmpty( $version_1 ); + $this->assertNotEmpty( $version_2 ); + $this->assertNotSame( $version_1, $version_2, 'Two same-minute saves with different content must produce different versions.' ); + } + + /** + * Anti-reversion guard: the legacy date-based version must not creep back into save_settings(). + * + * @covers FrmStyle::save_settings + */ + public function test_save_settings_does_not_reintroduce_legacy_date_based_version() { + $method = new ReflectionMethod( 'FrmStyle', 'save_settings' ); + $lines = file( $method->getFileName() ); + $body = implode( '', array_slice( $lines, $method->getStartLine() - 1, $method->getEndLine() - $method->getStartLine() + 1 ) ); + + $this->assertStringNotContainsString( 'njGi', $body, 'save_settings() must not reintroduce the legacy gmdate( "njGi" ) cache-busting version.' ); + } + + /** + * Regression guard: because the version is a content hash, publishing it for a write that + * silently failed is unrecoverable (every later save of the same content reproduces the same + * hash and URL, permanently pinning a downstream cache to the stale file). This forces a real + * write failure -- via a genuine filesystem directory collision, not a mock -- and asserts + * frm_last_style_update is left completely untouched: neither overwritten nor deleted. + * + * @covers FrmStyle::save_settings + */ + public function test_save_settings_leaves_version_untouched_when_file_write_fails() { + add_filter( 'frm_add_css_to_uploads_dir', '__return_true' ); + + $uploads = wp_upload_dir(); + $blocked_path = $uploads['basedir'] . '/formidable'; + + // A previous save (in this test or another) may have already created this as a real + // directory. It only holds generated/cache data, so it is safe to clear it to guarantee a + // deterministic collision below. + $this->rmdir_recursive( $blocked_path ); + + try { + $this->assertNotFalse( file_put_contents( $blocked_path, 'not a directory' ), 'Test setup: failed to create the blocking file.' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + $this->assertTrue( is_file( $blocked_path ), 'Test setup assumption: the blocking path must be a plain file, not a directory, so FrmCreateFile cannot create its "formidable" subdirectory there.' ); + + update_option( 'frm_last_style_update', 'sentinel-untouched' ); + + $frm_style = new FrmStyle( 'default' ); + $frm_style->save_settings(); + + $this->assertSame( 'sentinel-untouched', get_option( 'frm_last_style_update' ), 'frm_last_style_update must be left untouched (not updated, not deleted) when the CSS file write fails.' ); + } finally { + remove_filter( 'frm_add_css_to_uploads_dir', '__return_true' ); + + if ( is_file( $blocked_path ) ) { + unlink( $blocked_path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.unlink_unlink + } + } + } + + /** + * @param string $path + * + * @return void + */ + private function rmdir_recursive( $path ) { + if ( is_file( $path ) ) { + unlink( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.unlink_unlink + return; + } + + if ( ! is_dir( $path ) ) { + return; + } + + foreach ( array_diff( scandir( $path ), array( '.', '..' ) ) as $item ) { // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.WP.AlternativeFunctions.file_system_operations_scandir + $this->rmdir_recursive( $path . '/' . $item ); + } + + rmdir( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir + } + + /** + * The version must never advertise content the AJAX fallback ( frmpro_css option/transient ) + * is not already serving. Whenever save_settings() advances the version, frmpro_css must + * already be populated with exactly the content that hash was derived from. + * + * @covers FrmStyle::save_settings + */ + public function test_frmpro_css_is_populated_before_version_advances() { + delete_option( 'frm_last_style_update' ); + delete_option( 'frmpro_css' ); + delete_transient( 'frmpro_css' ); + + $frm_style = new FrmStyle( 'default' ); + $frm_style->save_settings(); + + $version = get_option( 'frm_last_style_update' ); + $this->assertNotEmpty( $version, 'Test setup assumption: the write should succeed and the version should advance in this environment.' ); + + $stored_css = get_option( 'frmpro_css' ); + $transient_css = get_transient( 'frmpro_css' ); + + $this->assertNotEmpty( $stored_css, 'The frmpro_css option must be populated whenever the version advances.' ); + $this->assertNotEmpty( $transient_css, 'The frmpro_css transient must be populated whenever the version advances.' ); + $this->assertSame( $stored_css, $transient_css, 'The frmpro_css option and transient must agree.' ); + $this->assertSame( + $version, + substr( md5( $stored_css ), 0, 12 ), + 'The advanced version must correspond exactly to the frmpro_css content already stored, so the enqueued URL and the AJAX fallback can never disagree.' + ); + } } diff --git a/tests/phpunit/styles/test_FrmStylesController.php b/tests/phpunit/styles/test_FrmStylesController.php index d4061d4bdb..b6e08d5bee 100644 --- a/tests/phpunit/styles/test_FrmStylesController.php +++ b/tests/phpunit/styles/test_FrmStylesController.php @@ -80,4 +80,66 @@ public function test_save() { $updated_style = $frm_style->get_one(); $this->assertSame( $style->post_title . ' Updated', $updated_style->post_title ); } + + /** + * Integration test: saving a style, reading the enqueued `formidable` handle's version, + * changing a colour and saving again must change the enqueued version. This is what actually + * busts a third-party cache keyed on the enqueued stylesheet URL. + * + * @covers FrmStylesController::get_css_version + * @covers FrmStylesController::enqueue_css + * @covers FrmStyle::save_settings + */ + public function test_css_version_changes_when_style_content_changes_after_save() { + $this->set_current_user_to_1(); + $this->set_front_end(); + + $frm_style = new FrmStyle( 'default' ); + $style = $frm_style->get_one(); + + $_POST = array( + 'ID' => $style->ID, + 'style_name' => $style->post_name, + 'frm_style' => wp_create_nonce( 'frm_style_nonce' ), + 'frm_action' => 'save', + 'frm_style_setting' => array( + 'post_title' => $style->post_title, + 'post_content' => array_merge( $style->post_content, array( 'submit_bg_color' => '112233' ) ), + ), + ); + + FrmStylesController::save_style(); + $version_1 = $this->get_registered_formidable_css_version(); + $this->assertNotEmpty( $version_1 ); + + // Change a colour and save again. + $_POST['frm_style_setting']['post_content'] = array_merge( $style->post_content, array( 'submit_bg_color' => '445566' ) ); + FrmStylesController::save_style(); + $version_2 = $this->get_registered_formidable_css_version(); + + $this->assertNotEmpty( $version_2 ); + $this->assertNotSame( $version_1, $version_2, 'The enqueued stylesheet version should change when the generated CSS content changes.' ); + } + + /** + * Force the `formidable` style handle to be re-registered so its version reflects the + * current `frm_last_style_update` option, then return that version. + * + * @return string + */ + private function get_registered_formidable_css_version() { + global $wp_styles, $frm_vars; + + $frm_vars['css_loaded'] = false; + + if ( isset( $wp_styles->registered['formidable'] ) ) { + wp_deregister_style( 'formidable' ); + } + + FrmStylesController::enqueue_css( 'register', true ); + + $this->assertArrayHasKey( 'formidable', $wp_styles->registered, 'The formidable stylesheet was not registered' ); + + return $wp_styles->registered['formidable']->ver; + } } From 62defb1c622272216c3e93b841111ac72e1b6853 Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 14 Aug 2026 18:22:06 +0100 Subject: [PATCH 3/7] test: assert the version against content, not the wall clock Answers CodeRabbit's review comment on tests/phpunit/styles/test_FrmStyle.php (the minute-boundary assertion at the old lines 246-255). test_same_minute_saves_with_different_content_produce_different_versions opened with `$before = time()`, closed with `$after = time()`, and asserted that gmdate( 'njGi', $before ) === gmdate( 'njGi', $after ). That is a test setup assumption asserted as a hard failure: two consecutive in-process calls normally land in the same minute, but nothing stops them straddling a minute boundary, and when they do the build goes red while the product is behaving correctly. The assertions that follow never needed the premise -- the version is a content hash, so it distinguishes the two saves regardless of when they happen. Rather than just delete the check (which would leave the test a duplicate of test_update_css_version_is_sensitive_to_content_in_both_directions), the clock-independence claim is now asserted directly: each version must equal substr( md5( $css ), 0, 12 ) for its own content. No minute-granularity value can satisfy both assertions, so the reversion the old check was groping at is now caught deterministically instead of only on a lucky run. Renamed and the docblock rewritten to match, since "same minute" is no longer the premise. Tests only -- no product code changed. Co-Authored-By: Claude Opus 5 (1M context) --- tests/phpunit/styles/test_FrmStyle.php | 37 +++++++++++++++++--------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/phpunit/styles/test_FrmStyle.php b/tests/phpunit/styles/test_FrmStyle.php index 8c638d014b..8c065591bf 100644 --- a/tests/phpunit/styles/test_FrmStyle.php +++ b/tests/phpunit/styles/test_FrmStyle.php @@ -233,30 +233,41 @@ public function test_update_css_version_is_sensitive_to_content_in_both_directio } /** - * Two saves within the same minute (in fact, within the same PHP process) with different - * content must produce different versions. This fails by construction against the legacy - * gmdate( 'njGi' ) implementation, which is minute-granularity and would store the identical - * value for both saves. + * Two consecutive saves in the same PHP process with different content must produce different + * versions, and each version must be derived from that content alone. This fails by + * construction against the legacy gmdate( 'njGi' ) implementation, which is minute-granularity + * and would store the identical value for both saves. + * + * The clock-independence claim is asserted against the content, not against the wall clock. An + * earlier form of this test first sanity-checked that both saves landed in the same legacy + * gmdate( 'njGi' ) bucket, which fails whenever two consecutive calls happen to straddle a + * minute boundary -- a red build for correct product behaviour, on a premise the assertions + * below do not actually need. Pinning each version to the hash of its own content proves the + * property outright rather than relying on the run being lucky. * * @covers FrmStyle::update_css_version */ - public function test_same_minute_saves_with_different_content_produce_different_versions() { + public function test_consecutive_saves_with_different_content_produce_content_derived_versions() { delete_option( 'frm_last_style_update' ); - $before = time(); - $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#aaaaaa}' ) ); + $css_1 = 'body{color:#aaaaaa}'; + $css_2 = 'body{color:#bbbbbb}'; + + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( $css_1 ) ); $version_1 = get_option( 'frm_last_style_update' ); - $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#bbbbbb}' ) ); + $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( $css_2 ) ); $version_2 = get_option( 'frm_last_style_update' ); - $after = time(); - - // Sanity check that this test actually exercises the same-minute case it claims to. - $this->assertSame( gmdate( 'njGi', $before ), gmdate( 'njGi', $after ), 'Test setup assumption: both saves happened within the same legacy gmdate( "njGi" ) bucket.' ); $this->assertNotEmpty( $version_1 ); $this->assertNotEmpty( $version_2 ); - $this->assertNotSame( $version_1, $version_2, 'Two same-minute saves with different content must produce different versions.' ); + $this->assertNotSame( $version_1, $version_2, 'Two consecutive saves with different content must produce different versions.' ); + + // Each version is the hash of its own content and nothing else. No minute-granularity + // value -- or any other clock-derived one -- can satisfy both of these assertions, which + // is the reversion this test exists to catch. + $this->assertSame( substr( md5( $css_1 ), 0, 12 ), $version_1, 'The version must be derived from the stylesheet content alone.' ); + $this->assertSame( substr( md5( $css_2 ), 0, 12 ), $version_2, 'The version must be derived from the stylesheet content alone.' ); } /** From a377edff21cc69063e93d2707d701f3bb4c642b7 Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 14 Aug 2026 21:20:48 +0100 Subject: [PATCH 4/7] test: prove both style-version tests are non-vacuous Answers CodeRabbit's two review comments on tests/phpunit/styles/test_FrmStyle.php. test_save_settings_leaves_version_untouched_when_file_write_fails could pass without ever reaching FrmCreateFile::create_file(): save_settings() returns early when css/custom_theme.css.php is missing, and the sentinel option would then be untouched for the wrong reason. It now asserts the source stylesheet exists before the call, and that frmpro_css was populated after it. That option is only written once create_file() has returned, so a populated frmpro_css alongside an untouched frm_last_style_update proves the write really was attempted and really did report failure. test_frmpro_css_is_populated_before_version_advances asserted an ordering claim only against the end state, which a regression that advances the version first and stores the CSS afterwards would still satisfy. It now observes the state at the version-write boundary via pre_update_option_frm_last_style_update and asserts frmpro_css (option and transient) already holds exactly the content that hash was derived from. The end-state assertions are kept. Also applies php-cs-fixer's blank-line-before-statement fix to three foreach blocks added earlier in this PR, and keeps the new lines under the 180-char PHPCS limit. Co-Authored-By: Claude Opus 5 (1M context) --- tests/phpunit/styles/test_FrmStyle.php | 61 +++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/tests/phpunit/styles/test_FrmStyle.php b/tests/phpunit/styles/test_FrmStyle.php index 8c065591bf..2f92c49a50 100644 --- a/tests/phpunit/styles/test_FrmStyle.php +++ b/tests/phpunit/styles/test_FrmStyle.php @@ -176,6 +176,7 @@ public function test_update_css_version_does_not_collide_across_previously_colli ); $legacy_versions = array(); + foreach ( $colliding_timestamps as $timestamp ) { $legacy_versions[] = gmdate( 'njGi', $timestamp ); } @@ -192,6 +193,7 @@ public function test_update_css_version_does_not_collide_across_previously_colli // Distinct content "saved" at each of those colliding moments must produce distinct // versions. This is the property the legacy format could not provide. $distinct_versions = array(); + foreach ( $colliding_timestamps as $index => $timestamp ) { $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{--collision-check:' . $index . '}' ) ); $distinct_versions[] = get_option( 'frm_last_style_update' ); @@ -202,6 +204,7 @@ public function test_update_css_version_does_not_collide_across_previously_colli // Identical content "saved" at each of those same moments must produce the SAME version, // proving the derivation is clock-independent rather than merely higher resolution. $stable_versions = array(); + foreach ( $colliding_timestamps as $timestamp ) { $this->run_private_method( array( 'FrmStyle', 'update_css_version' ), array( 'body{color:#123456}' ) ); $stable_versions[] = get_option( 'frm_last_style_update' ); @@ -290,6 +293,15 @@ public function test_save_settings_does_not_reintroduce_legacy_date_based_versio * write failure -- via a genuine filesystem directory collision, not a mock -- and asserts * frm_last_style_update is left completely untouched: neither overwritten nor deleted. * + * The sentinel assertion on its own would also hold if save_settings() never got as far as + * FrmCreateFile::create_file() -- it returns early when css/custom_theme.css.php is missing, + * which would make this test pass without exercising the failed write at all. Two guards + * close that off: the source stylesheet is asserted to exist before the call, and frmpro_css + * is asserted to have been populated afterwards. The option is only written after + * create_file() has returned, so a populated frmpro_css alongside an untouched version proves + * the write was attempted, reported failure, and the version write was skipped for that + * reason. + * * @covers FrmStyle::save_settings */ public function test_save_settings_leaves_version_untouched_when_file_write_fails() { @@ -306,12 +318,21 @@ public function test_save_settings_leaves_version_untouched_when_file_write_fail try { $this->assertNotFalse( file_put_contents( $blocked_path, 'not a directory' ), 'Test setup: failed to create the blocking file.' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents $this->assertTrue( is_file( $blocked_path ), 'Test setup assumption: the blocking path must be a plain file, not a directory, so FrmCreateFile cannot create its "formidable" subdirectory there.' ); + $this->assertTrue( + is_file( FrmAppHelper::plugin_path() . '/css/custom_theme.css.php' ), + 'Test setup assumption: the source stylesheet must exist, or save_settings() returns before FrmCreateFile::create_file() and this test asserts nothing.' + ); update_option( 'frm_last_style_update', 'sentinel-untouched' ); + delete_option( 'frmpro_css' ); $frm_style = new FrmStyle( 'default' ); $frm_style->save_settings(); + $this->assertNotEmpty( + get_option( 'frmpro_css' ), + 'The generated CSS must have been stored, which only happens once FrmCreateFile::create_file() has been called -- otherwise the failed write was never exercised.' + ); $this->assertSame( 'sentinel-untouched', get_option( 'frm_last_style_update' ), 'frm_last_style_update must be left untouched (not updated, not deleted) when the CSS file write fails.' ); } finally { remove_filter( 'frm_add_css_to_uploads_dir', '__return_true' ); @@ -349,6 +370,13 @@ private function rmdir_recursive( $path ) { * is not already serving. Whenever save_settings() advances the version, frmpro_css must * already be populated with exactly the content that hash was derived from. * + * This is an ordering claim, so it is asserted at the ordering boundary rather than after the + * fact: a pre_update_option_frm_last_style_update filter records what frmpro_css held at the + * instant the version write was attempted. Checking only the end state would let a regression + * that advances the version first and stores the CSS afterwards pass, since the final values + * agree either way. The end-state assertions are kept as well, so both the ordering and the + * resulting consistency are covered. + * * @covers FrmStyle::save_settings */ public function test_frmpro_css_is_populated_before_version_advances() { @@ -356,12 +384,41 @@ public function test_frmpro_css_is_populated_before_version_advances() { delete_option( 'frmpro_css' ); delete_transient( 'frmpro_css' ); - $frm_style = new FrmStyle( 'default' ); - $frm_style->save_settings(); + $observed = array(); + $observer = function ( $value ) use ( &$observed ) { + $observed[] = array( + 'version' => $value, + 'option' => get_option( 'frmpro_css' ), + 'transient' => get_transient( 'frmpro_css' ), + ); + + return $value; + }; + + add_filter( 'pre_update_option_frm_last_style_update', $observer ); + + try { + $frm_style = new FrmStyle( 'default' ); + $frm_style->save_settings(); + } finally { + remove_filter( 'pre_update_option_frm_last_style_update', $observer ); + } $version = get_option( 'frm_last_style_update' ); $this->assertNotEmpty( $version, 'Test setup assumption: the write should succeed and the version should advance in this environment.' ); + $this->assertCount( 1, $observed, 'save_settings() must write the version exactly once when the file write succeeds.' ); + + $at_write = $observed[0]; + $this->assertNotEmpty( $at_write['option'], 'The frmpro_css option must already be populated at the moment the version is written, not afterwards.' ); + $this->assertNotEmpty( $at_write['transient'], 'The frmpro_css transient must already be populated at the moment the version is written, not afterwards.' ); + $this->assertSame( + $at_write['version'], + substr( md5( $at_write['option'] ), 0, 12 ), + 'The version being written must hash the CSS already in frmpro_css, so the URL never advertises content the fallback is not serving.' + ); + $this->assertSame( $at_write['option'], $at_write['transient'], 'The frmpro_css option and transient must already agree at the moment the version is written.' ); + $stored_css = get_option( 'frmpro_css' ); $transient_css = get_transient( 'frmpro_css' ); From 506f5e59bf3fb61be23c1eb11cbb7ae7707a784b Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 14 Aug 2026 21:35:20 +0100 Subject: [PATCH 5/7] test: assert the version-write ordering for every write, not for one a377edff2 asserted save_settings() writes the version exactly once, which failed on PHP 8 in CI: it can write twice. get_css_content() renders custom_theme.css.php, which reads FrmStyle::get_all(), which creates a default style and calls update( 'default' ) -- and so save_settings() again -- when no style rows exist yet. Whether that re-entrant save happens depends on the state the rest of the suite leaves behind, so the count was never the right thing to pin. The ordering invariant is unaffected: each save_settings() stores frmpro_css before writing its own version. So assert that invariant for every observed write instead of for a single expected one, with the write index in the failure message. This is stronger than the original check, not weaker -- a regression that advanced the version ahead of the CSS in any of the writes now fails. Co-Authored-By: Claude Opus 5 (1M context) --- tests/phpunit/styles/test_FrmStyle.php | 34 ++++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/tests/phpunit/styles/test_FrmStyle.php b/tests/phpunit/styles/test_FrmStyle.php index 2f92c49a50..239de4e9ea 100644 --- a/tests/phpunit/styles/test_FrmStyle.php +++ b/tests/phpunit/styles/test_FrmStyle.php @@ -377,6 +377,13 @@ private function rmdir_recursive( $path ) { * agree either way. The end-state assertions are kept as well, so both the ordering and the * resulting consistency are covered. * + * The invariant is asserted for every observed write rather than for a single expected one. + * save_settings() can legitimately run more than once per call: get_css_content() renders + * custom_theme.css.php, which reads FrmStyle::get_all(), which creates a default style and + * calls update( 'default' ) -- and therefore save_settings() again -- when no style rows exist + * yet. How many writes happen is incidental and depends on the state the suite leaves behind; + * that each one is ordered after its own frmpro_css write is the property that matters. + * * @covers FrmStyle::save_settings */ public function test_frmpro_css_is_populated_before_version_advances() { @@ -407,17 +414,24 @@ public function test_frmpro_css_is_populated_before_version_advances() { $version = get_option( 'frm_last_style_update' ); $this->assertNotEmpty( $version, 'Test setup assumption: the write should succeed and the version should advance in this environment.' ); - $this->assertCount( 1, $observed, 'save_settings() must write the version exactly once when the file write succeeds.' ); + $this->assertNotEmpty( $observed, 'The version write must have been observed, or this test asserts nothing about the ordering.' ); - $at_write = $observed[0]; - $this->assertNotEmpty( $at_write['option'], 'The frmpro_css option must already be populated at the moment the version is written, not afterwards.' ); - $this->assertNotEmpty( $at_write['transient'], 'The frmpro_css transient must already be populated at the moment the version is written, not afterwards.' ); - $this->assertSame( - $at_write['version'], - substr( md5( $at_write['option'] ), 0, 12 ), - 'The version being written must hash the CSS already in frmpro_css, so the URL never advertises content the fallback is not serving.' - ); - $this->assertSame( $at_write['option'], $at_write['transient'], 'The frmpro_css option and transient must already agree at the moment the version is written.' ); + foreach ( $observed as $index => $at_write ) { + $where = ' (version write ' . ( $index + 1 ) . ' of ' . count( $observed ) . ')'; + + $this->assertNotEmpty( $at_write['option'], 'The frmpro_css option must already be populated at the moment the version is written, not afterwards.' . $where ); + $this->assertNotEmpty( $at_write['transient'], 'The frmpro_css transient must already be populated at the moment the version is written, not afterwards.' . $where ); + $this->assertSame( + $at_write['version'], + substr( md5( $at_write['option'] ), 0, 12 ), + 'The version being written must hash the CSS already in frmpro_css, so the URL never advertises content the fallback is not serving.' . $where + ); + $this->assertSame( + $at_write['option'], + $at_write['transient'], + 'The frmpro_css option and transient must already agree at the moment the version is written.' . $where + ); + } $stored_css = get_option( 'frmpro_css' ); $transient_css = get_transient( 'frmpro_css' ); From 828dc5b42bebf5908038894c1bf2ae6289666c9f Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Thu, 20 Aug 2026 15:36:17 +0100 Subject: [PATCH 6/7] style: satisfy PHPCS and PHP-CS-Fixer on the new style-version tests The `run analysis` label was missing from this PR, so PHPCS, Rector, PHP-CS-Fixer and the PHP syntax matrix were all skipping rather than passing. Adding the label surfaced two genuine failures in this diff: - PHP-CS-Fixer (phpdoc_separation): `update_css_version()`'s docblock needs a blank line between @param and @return. - PHPCS: the two new tests write to disk to exercise the failed-write path, which trips the VIP filesystem sniffs, plus assorted alignment and line-length violations. The filesystem writes are deliberate -- the tests cannot exercise a failed CSS write without a real file on disk -- so they are excluded in phpcs.xml per the existing precedent for test_FrmCSVExportHelper.php rather than suppressed inline. Everything else is formatting only; no test logic changed. Co-Authored-By: Claude Opus 5 (1M context) --- classes/models/FrmStyle.php | 1 + phpcs.xml | 10 ++++++++++ tests/phpunit/misc/test_FrmCreateFile.php | 24 ++++++++++++----------- tests/phpunit/styles/test_FrmStyle.php | 24 +++++++++++++++++------ 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/classes/models/FrmStyle.php b/classes/models/FrmStyle.php index 60bdea2ce4..a790313de0 100644 --- a/classes/models/FrmStyle.php +++ b/classes/models/FrmStyle.php @@ -450,6 +450,7 @@ public function save_settings() { * @since x.x * * @param string $css Generated stylesheet contents. + * * @return void */ private static function update_css_version( $css ) { diff --git a/phpcs.xml b/phpcs.xml index 00d2870cfe..b0787717c8 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -202,6 +202,16 @@ test_FrmCSVExportHelper.php + test_FrmCreateFile.php + test_FrmStyle.php + + + test_FrmCreateFile.php + test_FrmStyle.php + + + test_FrmCreateFile.php + test_FrmStyle.php FrmEntriesController.php diff --git a/tests/phpunit/misc/test_FrmCreateFile.php b/tests/phpunit/misc/test_FrmCreateFile.php index c523ad7364..746307384b 100644 --- a/tests/phpunit/misc/test_FrmCreateFile.php +++ b/tests/phpunit/misc/test_FrmCreateFile.php @@ -38,10 +38,9 @@ public function test_create_file_returns_true_on_successful_write() { ) ); - $content = 'body{color:#123456}'; - $result = $create_file->create_file( $content ); - - $written_path = $uploads['basedir'] . '/' . $folder . '/' . $file_name; + $content = 'body{color:#123456}'; + $result = $create_file->create_file( $content ); + $written_path = $uploads['basedir'] . '/' . $folder . '/' . $file_name; $this->paths_to_clean_up[] = $written_path; $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder . '/index.php'; $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder; @@ -83,11 +82,13 @@ public function test_create_file_returns_false_without_permission() { * @covers FrmCreateFile::create_file */ public function test_create_file_returns_false_when_directory_cannot_be_created() { - $uploads = wp_upload_dir(); - $blocking = 'frm-test-blocking-' . wp_generate_password( 8, false ); - $blocked_path = $uploads['basedir'] . '/' . $blocking; + $blocking = 'frm-test-blocking-' . wp_generate_password( 8, false ); + $blocked_path = wp_upload_dir()['basedir'] . '/' . $blocking; - $this->assertNotFalse( file_put_contents( $blocked_path, 'not a directory' ), 'Test setup: failed to create the blocking file.' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + $this->assertNotFalse( + file_put_contents( $blocked_path, 'not a directory' ), // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + 'Test setup: failed to create the blocking file.' + ); $this->paths_to_clean_up[] = $blocked_path; $create_file = new FrmCreateFile( @@ -126,7 +127,7 @@ public function test_append_file_and_combine_files_are_unaffected_by_the_widened $append_result_2 = $append_target->append_file( 'second-part;' ); $this->assertNull( $append_result_2 ); - $appended_path = $uploads['basedir'] . '/' . $folder . '/append.css'; + $appended_path = $uploads['basedir'] . '/' . $folder . '/append.css'; $this->paths_to_clean_up[] = $appended_path; $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder . '/index.php'; $this->paths_to_clean_up[] = $uploads['basedir'] . '/' . $folder; @@ -150,9 +151,10 @@ public function test_append_file_and_combine_files_are_unaffected_by_the_widened $combine_result = $combine_target->combine_files( array( $source_a, $source_b ) ); $this->assertNull( $combine_result, 'combine_files() must remain void regardless of create_file()\'s widened return type.' ); - $combined_path = $uploads['basedir'] . '/' . $folder . '/combined.css'; + $combined_path = $uploads['basedir'] . '/' . $folder . '/combined.css'; $this->paths_to_clean_up[] = $combined_path; - $this->assertSame( "a{color:#111}\nb{color:#222}\n", file_get_contents( $combined_path ) ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + $this->assertSame( "a{color:#111}\nb{color:#222}\n", file_get_contents( $combined_path ) ); } } diff --git a/tests/phpunit/styles/test_FrmStyle.php b/tests/phpunit/styles/test_FrmStyle.php index 239de4e9ea..ebbd729aec 100644 --- a/tests/phpunit/styles/test_FrmStyle.php +++ b/tests/phpunit/styles/test_FrmStyle.php @@ -307,8 +307,7 @@ public function test_save_settings_does_not_reintroduce_legacy_date_based_versio public function test_save_settings_leaves_version_untouched_when_file_write_fails() { add_filter( 'frm_add_css_to_uploads_dir', '__return_true' ); - $uploads = wp_upload_dir(); - $blocked_path = $uploads['basedir'] . '/formidable'; + $blocked_path = wp_upload_dir()['basedir'] . '/formidable'; // A previous save (in this test or another) may have already created this as a real // directory. It only holds generated/cache data, so it is safe to clear it to guarantee a @@ -316,8 +315,15 @@ public function test_save_settings_leaves_version_untouched_when_file_write_fail $this->rmdir_recursive( $blocked_path ); try { - $this->assertNotFalse( file_put_contents( $blocked_path, 'not a directory' ), 'Test setup: failed to create the blocking file.' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents - $this->assertTrue( is_file( $blocked_path ), 'Test setup assumption: the blocking path must be a plain file, not a directory, so FrmCreateFile cannot create its "formidable" subdirectory there.' ); + $this->assertNotFalse( + file_put_contents( $blocked_path, 'not a directory' ), // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents + 'Test setup: failed to create the blocking file.' + ); + $this->assertTrue( + is_file( $blocked_path ), + 'Test setup assumption: the blocking path must be a plain file, not a directory,' + . ' so FrmCreateFile cannot create its "formidable" subdirectory there.' + ); $this->assertTrue( is_file( FrmAppHelper::plugin_path() . '/css/custom_theme.css.php' ), 'Test setup assumption: the source stylesheet must exist, or save_settings() returns before FrmCreateFile::create_file() and this test asserts nothing.' @@ -333,7 +339,12 @@ public function test_save_settings_leaves_version_untouched_when_file_write_fail get_option( 'frmpro_css' ), 'The generated CSS must have been stored, which only happens once FrmCreateFile::create_file() has been called -- otherwise the failed write was never exercised.' ); - $this->assertSame( 'sentinel-untouched', get_option( 'frm_last_style_update' ), 'frm_last_style_update must be left untouched (not updated, not deleted) when the CSS file write fails.' ); + $this->assertSame( + 'sentinel-untouched', + get_option( 'frm_last_style_update' ), + 'frm_last_style_update must be left untouched (not updated, not deleted)' + . ' when the CSS file write fails.' + ); } finally { remove_filter( 'frm_add_css_to_uploads_dir', '__return_true' ); @@ -358,7 +369,8 @@ private function rmdir_recursive( $path ) { return; } - foreach ( array_diff( scandir( $path ), array( '.', '..' ) ) as $item ) { // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.WP.AlternativeFunctions.file_system_operations_scandir + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.WP.AlternativeFunctions.file_system_operations_scandir + foreach ( array_diff( scandir( $path ), array( '.', '..' ) ) as $item ) { $this->rmdir_recursive( $path . '/' . $item ); } From a8ed9aef1333cf5c9ba280862c38c7fac2e75806 Mon Sep 17 00:00:00 2001 From: Nathanael Jones Date: Fri, 21 Aug 2026 21:34:33 +0100 Subject: [PATCH 7/7] Suppress the DeepSource md5() false positive on the cache-version hash DeepSource: PHP fails this PR on PHP-A1004 ("Use of insecure md5() function") at the new update_css_version() call. It is a false positive: the value is a 12-character content-derived cache-busting token written to frm_last_style_update, not a password. The check's suggested remedy, password_hash(), is salted and non-deterministic, so it would defeat the entire mechanism -- the whole point is that identical stylesheet bytes must reproduce an identical version string. Suppressed at the line rather than repo-wide, following the existing skipcq convention in this codebase (classes/views/frm-entries/show.php, classes/views/frm-fields/back-end/field-options.php, and the phpcs-sniffs CommentSpacingSniff which explicitly whitelists skipcq: comments). Note that classes/models/ already contains seven md5()-for-cache-key calls of exactly this kind (FrmAddon, FrmAntiSpam, FrmApplicationApi, FrmFormApi, FrmFormTemplateApi, FrmStyleApi, FrmUsage), none annotated -- DeepSource only reports on lines the diff touches, which is why this one surfaced and those did not. The remaining DeepSource findings on this PR are all in tests/phpunit/, which is what #3269 addresses; this commit does not duplicate that change. Verified: php -l clean; the custom Formidable.Commenting.CommentSpacing sniff passes on the file; php-cs-fixer --dry-run --allow-risky=yes exits 0 against the LF form CI analyses (the working tree is CRLF via autocrlf, which cs-fixer reports on its own and is unrelated to this change). Co-Authored-By: Claude Opus 5 (1M context) --- classes/models/FrmStyle.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/classes/models/FrmStyle.php b/classes/models/FrmStyle.php index a790313de0..7402a18235 100644 --- a/classes/models/FrmStyle.php +++ b/classes/models/FrmStyle.php @@ -454,6 +454,11 @@ public function save_settings() { * @return void */ private static function update_css_version( $css ) { + // skipcq: PHP-A1004 -- md5() here is a content-derived cache-busting token, not a + // password hash. It must be deterministic so the same stylesheet always yields the + // same URL; password_hash() is salted and non-deterministic and would defeat the + // whole mechanism. Matches the existing md5()-for-cache-key calls in FrmAddon, + // FrmAntiSpam, FrmFormApi and FrmStyleApi. update_option( 'frm_last_style_update', substr( md5( $css ), 0, 12 ) ); }