Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Block theme updates if they have been modified by PM #104

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Block theme updates if they have been modified by PM #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
873c4da
Block theme updates if they have been modified by PM
johnstonphilip Mar 10, 2023
e1d8300
Merge branch 'main' of https://github.com/studiopress/pattern-manager…
johnstonphilip Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions wp-modules/pattern-data-handlers/pattern-data-handlers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ function update_pattern( $pattern ) {
FS_CHMOD_FILE
);

// Put a flag in the database to indicate this theme has been modified by PM.
update_option( 'pm_mod_' . get_stylesheet(), true );
Copy link
Contributor

@kienstra kienstra Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

If we're going to do this PR, would it be possible to move this into a helper function? So ideally it's only adding 1 line to update_pattern() and delete_pattern()


// TO DO: Fix issue with needing to "Save twice" on the frontend, because the pattern files are cached on the first save, making images on disk incorrect.

return $pattern_file_created;
Expand All @@ -331,6 +334,9 @@ function delete_pattern( string $pattern_name ): bool {
$result = $wp_filesystem && $wp_filesystem->exists( $pattern_path ) && $wp_filesystem->delete( $pattern_path );
tree_shake_theme_images( $wp_filesystem, 'copy_dir' );

// Put a flag in the database to indicate this theme has been modified by PM.
update_option( 'pm_mod_' . get_stylesheet(), true );

return $result;
}

Expand Down
44 changes: 44 additions & 0 deletions wp-modules/prevent-theme-updates/prevent-theme-updates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php
/**
* Module Name: Prevent Theme Updates
* Description: This module will prevent a theme from being updated if Pattern Manager has modified it.
* Namespace: PreventThemeUpdates
*
* @package pattern-manager
*/

declare(strict_types=1);

namespace PatternManager\PreventThemeUpdates;

// Exit if accessed directly.
if ( ! defined( 'ABSPATH' ) ) {
exit;
}

/**
* Prevent a theme update if it's been modified by Pattern Manager.
*
* @param mixed $update_themes_transient_data Value of site transient.
* @param string $transient Transient name.
* @return string
*/
function block_theme_updates_if_modified_by_pm( $update_themes_transient_data ) {

// No update object exists. Return early.
if ( empty( $update_themes_transient_data ) || ! isset( $update_themes_transient_data->response ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about removing this if, and simply blocking theme auto-updates if PM is active?

That'll probably anger some people.

But they're not going to benefit from PM anyway, and we could notify them on plugin activation that we're blocking theme auto-updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see the argument. Maybe a reason not to (and keep it how it is) is that someone might have another theme where they are testing update functionality with some custom server, and this would have them waste a lot of time trying to figure out why "their update code isn't working".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side, note: how many people are auto-updating in their local?

PM is for local development.

Copy link

Choose a reason for hiding this comment

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

I would offer that updates likely start in Local. So devs can test and make sure things are not breaking.

return $update_themes_transient_data;
}

// Loop through each theme that has an update available.
foreach ( $update_themes_transient_data->response as $theme_slug => $theme_with_update_available ) {
$theme_has_been_modified_by_pm = get_option( 'pm_mod_' . $theme_slug );

if ( $theme_has_been_modified_by_pm ) {
Copy link
Contributor

@kienstra kienstra Mar 13, 2023 •
edited

Choose a reason for hiding this comment

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

Hi @johnstonphilip,
What do you think about only blocking updates for the active theme when a pm_pattern post exists?

$theme_has_been_modified_by_pm = basename( get_stylesheet_directory() ) === $theme_slug && get_pm_post_ids();

As you know, editing a pattern creates a pm_pattern post.

Still, this will miss this case:

  1. Edit a pattern
  2. Activate another theme
  3. Go back to the theme from step 1
  4. Try to update the active theme via /wp-admin
  5. Expected: this code blocks the theme update
  6. Actual: it doesn't, as the pm_pattern posts were deleted on changing themes

This approach won't require saving any new data.

But your approach is much more complete.

Thanks for being so patient about this.

unset( $update_themes_transient_data->response[ $theme_slug ] );
}
}

return $update_themes_transient_data;
}
add_filter( 'site_transient_update_themes', __NAMESPACE__ . '\block_theme_updates_if_modified_by_pm', 10 );
95 changes: 95 additions & 0 deletions wp-modules/prevent-theme-updates/tests/PreventThemeUpdateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<?php
/**
* Class PatternManagerPreventThemeUpdateTest
*
* @package pattern-manager
*/

/**
* Test this module's functions.
*/
class PatternManagerPreventThemeUpdateTest extends WP_UnitTestCase {

/**
* Test that when themes do not have PM mods, their updates are not prevented.
*/
public function testDoNotBlockThemesWithNoPmMods() {
$value = new stdClass();
$value->last_checked = 1678397389;
$value->checked = array(
'twentytwentythree' => 1.1,
'twentytwentytwo' => 1.4,
);
$value->response = array(
'twentytwentythree' => array(
'theme' => 'twentytwentythree',
'new_version' => 1.0,
'url' => 'https://wordpress.org/themes/twentytwentythree/',
'package' => 'https://downloads.wordpress.org/theme/twentytwentythree.1.0.zip',
'requires' => 6.1,
'requires_php' => 5.6,
),
);
$value->no_update = array(
'twentytwentytwo' => array(
'theme' => 'twentytwentytwo',
'new_version' => 1.3,
'url' => 'https://wordpress.org/themes/twentytwentytwo/',
'package' => 'https://downloads.wordpress.org/theme/twentytwentytwo.1.3.zip',
'requires' => 5.9,
'requires_php' => 5.6,
),
);
$value->translations = array();

$expected = $value;

$result = \PatternManager\PreventThemeUpdates\block_theme_updates_if_modified_by_pm( $value, 'update_themes' );

$this->assertEquals( $expected, $result );
}


/**
* Test that when themes do have PM mods, their updates are prevented.
*/
public function testBlockThemesWithPmMods() {
update_option( 'pm_mod_twentytwentythree', true );

$value = new stdClass();
$value->last_checked = 1678397389;
$value->checked = array(
'twentytwentythree' => 1.1,
'twentytwentytwo' => 1.4,
);
$value->response = array(
'twentytwentythree' => array(
'theme' => 'twentytwentythree',
'new_version' => 1.0,
'url' => 'https://wordpress.org/themes/twentytwentythree/',
'package' => 'https://downloads.wordpress.org/theme/twentytwentythree.1.0.zip',
'requires' => 6.1,
'requires_php' => 5.6,
),
);
$value->no_update = array(
'twentytwentytwo' => array(
'theme' => 'twentytwentytwo',
'new_version' => 1.3,
'url' => 'https://wordpress.org/themes/twentytwentytwo/',
'package' => 'https://downloads.wordpress.org/theme/twentytwentytwo.1.3.zip',
'requires' => 5.9,
'requires_php' => 5.6,
),
);
$value->translations = array();

$expected = $value;
$expected->response = array();

$result = \PatternManager\PreventThemeUpdates\block_theme_updates_if_modified_by_pm( $value, 'update_themes' );
Copy link
Contributor

Choose a reason for hiding this comment

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

These are great unit tests. Very thorough, and very realistic data.


$this->assertEquals( $expected, $result );
}

}