Mulitsite Patch

[qstudio]

This patch adds a table lookup, which tries to take into consideration if the plugin is running on a multsite and if it is network active and return the correct table prefix - using either prefix or base_prefix.

[froger-me]

Hi @qstudio ! 

Thank you very much for the contribution!
I'd consider merging this PR if the following changes are added:

• Remove all extra comments (keep consistent with original)
• Remove all namespace (keep consistent with original)
• Remove extra _ from variable names after $ (keep consistent with original)
• Keep variable names consistent (example: always use the same variable name for the table after calling Wprus_Settings::get_wpdb_table( 'wprus_logs' ))
• Remove all debug code (calls to error_log and associated comments)
• Define a constant in wprus.php and use it as plugin base file value instead of using a variable passed in parameter for the constructor, and remove the extra property (keep consistent with original - see annex below)
• Use WPCS with WordPress-Extra standard and conform to code standards

Annex:

if ( ! defined( 'WPRUS_PLUGIN_FILE ) ) {
    define( 'WPRUS_PLUGIN_FILE', plugin_basename( __FILE__ ));
}

[qstudio]

Thanks for the detailed feedback, it all makes sense, however, I am not sure I'll have time to complete these updates myself - at least not for a good while - perhaps it would be easier for you to write up your own version of this patch, following the standards you define?

Our projects are running off a stable fork for now - the plugin itself is excellent - thanks!

[froger-me]

Hi @qstudio !

During development, Multisite support was not included because WPRUS was made as an alternative (distributed vs. centralized). I however do understand the value of a hybrid setup, hence why I'd be glad to include changes that bring about the possibility, provided these changes are submitted in an accessible way.

Requests for new feature are regularly submitted through other channels, and I refuse them because of the added workload. Yours has the huge advantage to be a PR (many thanks for that!), and I was under the impression that it meant we could get into the workflow of code review, consistency checks, etc. like PRs are designed for before we actually merge pull the request. Writing up my own version of the patch would defeat the purpose of the PR process.

I guess time is indeed a scarce resources for most of us who are willing to make code contribution for free, unfortunately :(.

[qstudio]

Hey! Yes.. I can only be honest.. I have my own OS projects which I barely find time to manage - if at all!.. this patch came as a result of a commercial project, which - sadly, but realistically - takes precedence, at least for now.

That all said, I think it is fair to make a contribution to an excellent piece of software, which is actually pretty central to the application we are writing - so I will try to persuade my client of the importance of getting out patch merged into the main branch, so that we can benefit from future updates - in which case, I can take a look at your request and produce a cleaner PR.

On a side note - and without remembering the specific line - there was one more issue I had to fix, there was a hash / check failure when comparing the passed domain and the stored domain value - at least when running on a localhost - while the domain was clearly https, the stored protocol was http and the check failed - perhaps the protocol is not so important for this domain validation and could be emitted?

[damalis]

Hi @qstudio

I am getting this error; _Fatal error: Uncaught Error: Class 'Wprus_Settings' not found in /var/www/html/wp-content/plugins/wp-remote-users-sync/inc/class-wprus.php:68 Stack trace: #0 /var/www/html/wp-content/plugins/wp-remote-users-sync/inc/class-wprus.php(30): Wprus::maybe_create_or_upgrade_db() #1 /var/www/html/wp-includes/class-wp-hook.php(303): Wprus::activate('') #2 /var/www/html/wp-includes/class-wp-hook.php(327): WP_Hook->apply_filters('', Array) #3 /var/www/html/wp-includes/plugin.php(470): WP_Hook->do_action(Array) #4 /var/www/html/wp-admin/plugins.php(193): do_action('activatewp-rem...') #5 {main} thrown in /var/www/html/wp-content/plugins/wp-remote-users-sync/inc/class-wprus.php on line 68

I fixed this error; in file wprus.php

-43 require_once WPRUS_PLUGIN_PATH . 'inc/class-wprus-settings.php'; +31 require_once WPRUS_PLUGIN_PATH . 'inc/class-wprus-settings.php';

[qstudio]

@erdalaltin - looking at the backtrace, this was triggered during plugin activation - is that correct?

[qstudio]

@froger-me perhaps the db table selector can have it's own class file which can be included on the activate and deactivate callbacks and wherever this logic is required before the main plugin classes are available?

[damalis]

@erdalaltin - looking at the backtrace, this was triggered during plugin activation - is that correct?

Yes, That right.

[qstudio]

@erdalaltin - Added an example of a seperate DB class, which works to allow plugin activation and deactivation, please can you check this works for you?

[damalis]

@erdalaltin - Added an example of a seperate DB class, which works to allow plugin activation and deactivation, please can you check this works for you?

İt works. Thanks...

[qstudio]

@froger-me - is this patch now in an acceptable format?

[froger-me]

Hi @qstudio ,

Thanks for all these changes, and thanks @erdalaltin for the follow-up.

Just looking at it at first glance, I'm afraid there is cleaning needed in several places as although the code may be functional, it does not pass WordPress Coding Standards.

Examples:

The signature's syntax is not WPCS valid ; moreover, the { should not be on a different line.

public static function get_table( string $table = null ):?string
{
// ...
}

becomes

public static function get_table( $table = null ) {
// ...
}

Equal operators should be aligned when doing variable assignment on multiple lines - using spaces

$url_parse_url = parse_url( $url );
$url_host = $url_parse_url['host'];

becomes

$url_parse_url = parse_url( $url );
$url_host      = $url_parse_url['host'];

Spaces are missing, some line returns superfluous:

        if( is_null( $table ) ){

            return null;

        }

becomes

        if ( is_null( $table ) ) {

            return null;
        }

and

        if( isset( self::$tables[$table] ) ){

becomes

        if ( isset( self::$tables[ $table ] ) ) {

etc, etc... I won't list it all here, but respect of WordPress Coding Standards is an absolute requirement for me to merge any pull request, as stated previously.
I would have thought this would have been taken care of first before adding new features.

Speaking of new features or alterations thereof, I do not wish to compromise with the protocol: https is to be required no matters what.

Moreover, there still are comments here and there that are not consistent with the rest of the plugin's code base, such as:

            // error_log( 'Returning cache for table: '.$table.' --> '. self::$tables[$table] );

                /** 
                 * note that in some cases, both the sending and master sites might use the same protocol ( for example https )
                 *  but the passed or master value might return a different protocol ( for example http )
                 * so, this fix is proposed to run the string comparison just against the domain part of the URL, not including the protocol
                 */

            // hack #####

(this one is a big code smell)

Lastly, there are still many occurrences of function calls marked with the global namespace \:

            $use_base_prefix = \apply_filters( 'wprus_wpdb_use_base_prefix', $use_base_prefix );

        if(
            \is_multisite()
        ){

etc, etc

I have little interest making the plugin Multisite-compatible on my own, because I do not work with Multisite. However, I still welcome pull requests and I am very thankful for the work, and I do wish to merge it, as long as the WordPress Coding Standards are respected, code has been tested so that is needs as little maintenance as possible, and absolutely no compromise in terms of security is made.

[qstudio]

I feel like I just got my homework back and it's covered in RED marks and notes telling me that I should try harder :)

As mentioned before, I am trying to squeeze this update in between a pretty busy schedule, and while I do admire your adherence to standards, I probably patched 10 WordPress plugins this week and not one of them used the same rules.. so it's not always easy to switch from one pattern to another.

You also did not answer the question, is the new database class allowed and acceptable?

[froger-me]

Trust me, I do understand the frustration. This is partially for this reason I put the code base on Github: if another developer or team feels like adding features following a different approach, forking is the way to go. a pull request means that anything that's merged will be for me to maintain - and because of that, I do not wish to add the extra workload of cleaning the code added at merge - it has to follow the same standard to remain consistent, giving me a level of control I am comfortable with ; indeed, switching to a different pattern (when talking WordPress, WordPress Coding Standards is the authoritative one, no matter what other devs think) is also an extra step I need to take to answer questions like "is the new database class allowed and acceptable?".

I did take that extra step just now because of the insistence - the idea seems acceptable. I wonder if it could not just be a function in functions.php with a simple function-scoped static variable instead, though: without testing it myself, I can't really estimate the impact of including a new file, using a class, and use a static property. In any case, nitpicking here would be falling for the trap of early optimization, so I think it's fine with this approach indeed.

Regarding the tone of my last reply: I very much try to make things clear for the sake of the plugin's code base, and it does indeed come across as being very direct. I do hope it is not taken personally, as it's merely with the intention of being goal-driven.

[qstudio]

All good, it's just some (old) school traumas coming up I guess! I mean, this is one of the forgotten benefits of OS, being pushed by other to up your own game.. I'll re-visit the patch when I have time..

Honestly, if there is not much benefit or even call for this add-on, you could also reject it, as you said - we can maintain it within our own fork?

[scratchmasterp]

This patch works at removing the database error! Please try to integrate. It would be great if this was part of the main plugin. It stops the database error and enables us to choose the user_meta boxes

[froger-me]

No activity from original PR author for more than a year. Feature to be implemented in v1.2.15, with major corrections (as requested above, and more) and significantly different design choices fom above. Closing the PR without merging.

[qstudio]

@froger-me happy to hear that the idea at least might make it into the core plugin, we no longer use this architectural approach, so I had no further commercial justification to work on the patch - good luck!