Adds a cast to http option, this is enabled by default on new install…#297
Conversation
…s, but false on existing installs.
WalkthroughAdds a new Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Settings/Settings.php`:
- Around line 563-574: The PHPDoc for should_cast_archived_to_https is
inconsistent: it claims the optional default is true while the method signature
sets bool $default_value = false; update the PHPDoc to reflect the actual
default (false) by changing the `@param` description to say Optional default, set
to false (or alternatively change the method signature to = true if the intended
default is true); edit the docblock above the should_cast_archived_to_https
function (and ensure any mention of CAST_ARCHIVED_TO_HTTPS usage remains
accurate).
🧹 Nitpick comments (2)
tests/Settings/Test_Settings.php (1)
278-295: Consider adding cleanup insetUp()for test isolation.The test correctly validates all scenarios for
should_cast_archived_to_https(). However, thesetUp()method (lines 25-35) doesn't deleteSettings::CAST_ARCHIVED_TO_HTTPS, which could cause test pollution if tests run in different orders.♻️ Suggested addition to setUp()
public function setUp(): void { parent::setUp(); delete_option( Settings::ALLOWED_POST_TYPES ); delete_option( Settings::DROP_TABLES_ON_UNINSTALL_KEY ); delete_option( Settings::MIGRATIONS_KEY ); delete_option( Settings::LINK_EXCLUSIONS ); delete_option( Settings::SCAN_EXISTING_POSTS ); delete_option( Settings::FIXER_OPTION ); + delete_option( Settings::CAST_ARCHIVED_TO_HTTPS ); update_option(Settings::PROCESS_LINKS, true); }src/Dashboard/Settings_Page.php (1)
944-968: Minor: Consider capitalizing "URLs" in the description text.The description text at line 965 uses lowercase "urls" which should be "URLs" for proper capitalization of the acronym.
📝 Suggested text fix
<p class="description"> - <?php esc_html_e( 'Enabling this will convert all archived urls from http to https.', 'internet-archive-wayback-machine-link-fixer' ); ?> + <?php esc_html_e( 'Enabling this will convert all archived URLs from HTTP to HTTPS.', 'internet-archive-wayback-machine-link-fixer' ); ?> </p>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Dashboard/Settings_Page.php`:
- Line 960: The call to Settings::should_cast_archived_to_https currently passes
an extraneous argument (checked( Settings::should_cast_archived_to_https( false
) )); remove the parameter so the call matches the method signature and other
usages—i.e., change the call to invoke Settings::should_cast_archived_to_https()
with no arguments inside the checked(...) invocation.
In `@tests/Settings/Test_Settings.php`:
- Around line 278-292: Update the test docblock to remove the incorrect mention
of "definable fallbacks" since Settings::should_cast_archived_to_https() does
not accept fallback parameters; specifically edit the docblock for
test_can_get_cast_to_https_option_with_fallbacks() to describe only the default
behavior and override via update_option, or rename the test to reflect absence
of fallbacks, and reference the Settings::should_cast_archived_to_https() method
in the description so it accurately matches the tested behavior.
🧹 Nitpick comments (2)
src/Link/Link.php (1)
281-285: Handle potentialnullreturn frompreg_replace.
preg_replace()can returnnullon error. While unlikely here given the simple pattern and validated input, the return value is assigned directly to$archived_hrefwhich is typed as?string, so this is acceptable. However, for defensive coding, consider adding a null coalesce.🛡️ Optional defensive fix
// If the setting to cast to https is enabled, cast the start of the url to https. if ( Settings::should_cast_archived_to_https() ) { // Replace http with https at the start of the url. - $archived_href = preg_replace( '#^http://web-wp\.archive\.org/#i', 'https://web-wp.archive.org/', $archived_href ); + $archived_href = preg_replace( '#^http://web-wp\.archive\.org/#i', 'https://web-wp.archive.org/', $archived_href ) ?? $archived_href; }src/Dashboard/Settings_Page.php (1)
944-968: Minor: Description text could clarify scope.The description says "convert all archived urls from http to https" but the implementation only converts
http://web-wp.archive.org/URLs. Consider clarifying this is specific to the Archive.org domain for accuracy.
…s, but false on existing installs.
Changes proposed in this Pull Request
This pull request introduces a new feature that allows users to force archived URLs to use HTTPS instead of HTTP. This is accomplished by adding a new setting to the plugin, updating the setup wizard to enable this option by default for new users, modifying the link handling logic to respect this setting, and providing comprehensive tests for the new functionality.
New HTTPS-forcing feature:
CAST_ARCHIVED_TO_HTTPSin theSettingsclass, and registered it as a boolean option in the settings page. [1] [2]Default behavior and onboarding:
Core logic changes:
Linkclass so that, when the new setting is enabled, archived URLs are rewritten to usehttps://web-wp.archive.org/instead ofhttp://web-wp.archive.org/.should_cast_archived_to_https()to theSettingsclass to retrieve the setting value, and ensured the setting is cleared with all other options when appropriate. [1] [2]Testing:
Testing instructions
Mentions #293
Summary by CodeRabbit
New Features
Tests