Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper file, includes/functions.php, providing a collection of utility functions for the MSLS plugin. The review identified several critical issues, including a potential XSS vulnerability due to unescaped output in msls_the_switcher, a lack of type safety and method existence checks when handling the msls_get_output filter, and inconsistent use of factory and singleton patterns for MslsOptions and MslsOptionsPost objects. Additionally, the msls_return_void function was flagged as redundant given the availability of WordPress's native __return_void() function.
| // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | ||
| echo msls_get_switcher( $arr ); |
There was a problem hiding this comment.
The phpcs:ignore comment indicates a known security issue. The msls_get_switcher function generates HTML that includes unescaped data (e.g., $link->txt in MslsOutput.php line 98). Directly echoing this output without proper escaping creates a Cross-Site Scripting (XSS) vulnerability. All output that might contain user-supplied or dynamic data should be escaped before being echoed.
echo wp_kses_post( msls_get_switcher( $arr ) );| $obj = apply_filters( 'msls_get_output', null ); | ||
|
|
||
| return ! is_null( $obj ) ? strval( $obj->set_tags( $arr ) ) : ''; |
There was a problem hiding this comment.
The apply_filters call on line 18 can return any type. If it returns a non-object or an object without a set_tags method, the subsequent call to $obj->set_tags() on line 20 will result in a fatal error. It's crucial to ensure that the return value of the filter is a valid object before attempting to call its methods.
$obj = apply_filters( 'msls_get_output', null );
if ( is_object( $obj ) && method_exists( $obj, 'set_tags' ) ) {
return strval( $obj->set_tags( $arr ) );
}
return '';| function msls_get_post( int $id ): \lloc\Msls\MslsOptionsPost { | ||
| return new \lloc\Msls\MslsOptionsPost( $id ); |
There was a problem hiding this comment.
The MslsOptions::create() factory method is designed to correctly instantiate MslsOptionsPost (and other MslsOptions subclasses) based on the current context. Directly calling new \lloc\Msls\MslsOptionsPost( $id ) bypasses this factory logic, which might include important setup or filtering. This could lead to unexpected behavior or missing functionality.
function msls_get_post( int $id ): \lloc\Msls\MslsOptionsPost {
return \lloc\Msls\MslsOptions::create( $id );
}| function msls_get_flag_url( string $locale ): string { | ||
| return ( new \lloc\Msls\MslsOptions() )->get_flag_url( $locale ); |
There was a problem hiding this comment.
The msls_options() function provides a singleton instance of MslsOptions. Directly instantiating new \lloc\Msls\MslsOptions() bypasses this singleton pattern, which can lead to inconsistent behavior or unnecessary object creation if MslsOptions has state or expensive initialization. For consistency and potential performance benefits, the singleton should be used.
function msls_get_flag_url( string $locale ): string {
return msls_options()->get_flag_url( $locale );
}| $options = \lloc\Msls\MslsOptions::create(); | ||
| $url = $blog->get_url( $options ); |
There was a problem hiding this comment.
Similar to msls_get_flag_url, this function directly instantiates \lloc\Msls\MslsOptions::create(). While create() is a factory, msls_options() provides a singleton instance for general use. Using msls_options() ensures consistency across the application and avoids potential issues if MslsOptions relies on a shared state.
$options = msls_options();
$url = $blog->get_url( $options );| function msls_return_void(): void { | ||
| } |
There was a problem hiding this comment.
The function msls_return_void is described as a "Trivial void function for actions that do not return anything." This function appears to be redundant. If its purpose is to serve as a no-op for WordPress hooks, the core WordPress function __return_void() should be used for consistency and to avoid unnecessary code. If it's called directly, it serves no functional purpose.
🔍 WordPress Plugin Check Report
📊 Report
|
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
86 |
WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn_post__not_in | Using exclusionary parameters, like post__not_in, in calls to get_posts() should be done with caution, see https://wpvip.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/ for more information. |
📁 includes/MslsMain.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
143 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_SAVE_ACTION". |
📁 includes/MslsPostTagClassic.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
39 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ADD_INPUT_ACTION". |
71 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_EDIT_INPUT_ACTION". |
📁 includes/Map/HrefLang.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
69 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_HEAD_HREFLANG_HOOK". |
📁 includes/ContentImport/ContentImporter.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
288 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_BEFORE_IMPORT_ACTION". |
349 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_AFTER_IMPORT_ACTION". |
📁 includes/Component/Input/Select.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
39 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::RENDER_FILTER". |
📁 includes/MslsOutput.php (6 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
93 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_GET_HOOK". |
122 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ALTERNATE_LINKS_HOOK". |
138 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ALTERNATE_LINKS_DEFAULT_HOOK". |
141 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ALTERNATE_LINKS_ARR_HOOK". |
154 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_NO_TRANSLATION_FOUND_HOOK". |
185 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_GET_TAGS_HOOK". |
📁 includes/MslsPostTag.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
116 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ADD_INPUT_ACTION". |
150 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_EDIT_INPUT_ACTION". |
📁 includes/MslsBlog.php (3 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
143 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_GET_PERMALINK_HOOK". |
147 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_GET_PERMALINK_HOOK". |
201 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::WP_ADMIN_BAR_SHOW_SITE_ICONS_HOOK". |
📁 includes/MslsWidget.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
58 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ALTERNATIVE_CONTENT_HOOK". |
📁 includes/MslsAdmin.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
242 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_REGISTER_ACTION". |
341 |
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "self::MSLS_ACTION_PREFIX . $section". |
🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check
No description provided.