Skip to content

clivezhg/Unreasonable-Code-Reviews

Folders and files

NameName
Last commit message
Last commit date

Latest commit

 

History

7 Commits
 
 

Repository files navigation

This special document might look a bit strange, it is mainly for explaining to the users what have happened to More-Lang, and some other people may find it useful to them (especially if having a hosted plugin, or interested in the ecosystem of Wordpress).

More-Lang was a highly rated Wordpress plugin before it was closed, it got the highest user review score (https://wordpress.org/support/plugin/more-lang/reviews/) in the hundreds of plugins which have significant users (i.e., >1000 installations) under the same category.

More-Lang was closed on 2021-07-09 without any warning. A few days later I consulted the team, it was said because my registered email address failed to receive an automated mail, I needed to register a new email to re-open the plugin. After the new email was accepted by the system, I received code reviews from the [plugins@wordpress.org] for re-opening the plugin.

The reviews went on for a few rounds. However, in my opinion, there's no benefit from the points of the reviewer, the recommended changes would even break the plugin logic and reduce the performance.

So, the reviews were unreasonable to me, the plugin was closed at last.

(P.S.. There had been several rounds of reviews when I initially submitted the plugin to the [wordpress.org/plugins], then the plugin was accepted and hosted. However, all the points of the last reviewer had existed in those initial hosted files.
And, nearly all the points of the last reviewer also existed in the Wordpress core, but one of the reviewer's theories is like "something can be done by Wordpress core, but cannot be done by you". However, because More-Lang is a multilingual plugin, it will make multilingual equivalents of Wordpress core, so the reviewer's theory is not suitable for More-Lang. If it's a irreconcilable situtation, the initial reviewer should reject in the begining.
So, either the last review is unreasonable, or the first review is unreasonable, as a whole the review team is untrustworthy & unpredictable.)

The following are some key review points:

(1). Regarding this line: https://github.com/clivezhg/more-lang/blob/dfbe046a6136965e1ffa85ecb3ad63079b6eedde/ext/ml_custom.php#L115

			add_post_meta( $object_id, $loc_meta_key, $_POST[$val_name] );

The reviewer said '$_POST[$val_name]' (which is a post_meta translation) must be sanitized.

However, in the Wordpress Core when saving post_meta, it doesn't do any sanitizing: https://github.com/WordPress/WordPress/blob/2cc9f735238212adb0bba7888e11eccb4bf91bc2/wp-admin/includes/ajax-actions.php#L1627-L1649
As said before, More-Lang is making multilingual equivalents of Wordpress Core, they must be the same formats. So any extra sanitizing than Wordpress Core is useless here, which may even break the business logic in some cases.
(And like other More-Lang translations, More-Lang will replace the Core post_meta value with the translated one after fetched from the DB using Wordpress filters ASAP, so it shall not introduce extra issue to Wordpress. I.e., the security is in the same level as Wordpress Core)

(2). Regarding this line: https://github.com/clivezhg/more-lang/blob/dfbe046a6136965e1ffa85ecb3ad63079b6eedde/inc/ml_pub.php#L209

			$ml_requested_url_locale = trim( $_GET['lang'] );

The reviewer said '$ml_requested_url_locale' must be sanitized.

Actually, in More-Lang, the '$ml_requested_url_locale' will be used to match a known set (e.g., it can only be "en" or "jp"), if it is not in the set, then "" will be assigned to '$ml_requested_url_locale'. This process is already a perfect sanitizing. Any extra sanitizing is nonsense, hard to understand, and will reduce the performance.
The matching & assigning process is here: https://github.com/clivezhg/more-lang/blob/dfbe046a6136965e1ffa85ecb3ad63079b6eedde/inc/ml_pub.php#L221-L229

(3). Regarding this line (and many similiar lines): https://github.com/clivezhg/more-lang/blob/dfbe046a6136965e1ffa85ecb3ad63079b6eedde/inc/ml_editor_postinp.php#L17

	<textarea id="content_<?php echo $mlocale; ?>" name="content_<?php echo $mlocale; ?>" class="ml-local-ta" style="display:none;"><?php

The reviewer required "escape every variable" when 'echo'.

Firstly, in general sense, this guideline is problematic. Because there's a very common coding pattern: store the html in a variable, change it, at last echo it, then 'esc...' will produce incorrect string.
Secondly, if someone wants to write malicious code when 'echo', it's very easy to disguise to bypass this guideline check. So, this guideline is basically useless.
Thirdly, there are many escaped 'echo's in More-Lang, and all unescaped are proved to be safe (i.e., their data sources are under controlled), to add 'esc...' to these safe 'echo's is useless and causing pefromance issue.
Lastly, I inspected around 10 popular plugins, none of them does "escape every variable". Then I inspected Wordpress Core, it even does much worse than More-Lang, it doesn't 'esc...' some uncontrolled data.
So, this guideline looks a little unrealistic (too awkward for most developers to implement), the reviewer was giving very very unfair task to More-Lang...but of course, the reviewer denied (without any reasonable explanation).

That's how More-Lan was closed. And as far as my inspection, regarding the review points, More-Lang is safer than Wordpress Core & most of the hosted plugins (which also widely violate the review team's "guidelines"), It looked that the reviewer closed it mainly because he/she desired to do, weaving variety of unreasonable reasons (otherwise most of the hosted plugins should be closed).

If you would like to comment this document or share your story, you can create issues in this project.

About

For Unreasonable Code Reviews discussion

Resources

Stars

Watchers

Forks

Releases

No releases published

Packages

No packages published