reduxframework / redux-framework

Redux is a simple, truly extensible options framework for WordPress themes and plugins!
http://redux.io
Other
1.74k stars 584 forks source link

Fix for ajax save and max_input_vars limit #2631

Closed harunbasic closed 8 years ago

harunbasic commented 9 years ago

Even if you are using serialized string when there are more options than max_input_vars allows there will be an error.

In the method ajax_save in the framework.php you are using parse_str() which depends on max_input_vars.

The code below isn't mine. I have found it and tested it. It works good and removes that limit which parse_str() adds. With this change Redux won't be limited by that.

All you have to do is replace the parse_str( $_POST['data'], $values ); with $values = redux_parse_str( $_POST['data'] );

/**
 * Parses the string into variables without the max_input_vars limitation.
 *
 * @since   1.0
 * @access  public
 * @param   string $string
 * @return  array $result
 */
function redux_parse_str( $string ) {

    if ( '' == $string ) {
        return false;
    }

    $result = array();
    $pairs = explode( '&', urldecode( $string ) );

    foreach ( $pairs as $key => $pair ) {

        // use the original parse_str() on each element
        parse_str( $pair, $params );

        $k=key( $params );

        if( ! isset( $result[$k] ) ) {
            $result+=$params;
        } else {
            $result[$k] = redux_array_merge_recursive_distinct( $result[$k], $params[$k] );
        }
    }

    return $result;
}

/**
 * Merge arrays without converting values with duplicate keys to arrays as array_merge_recursive does.
 *
 * As seen here http://php.net/manual/en/function.array-merge-recursive.php#92195
 *
 * @since   1.0
 * @access  public
 * @param   array $array1
 * @param   array $array2
 * @return  array $merged
 */
function redux_array_merge_recursive_distinct( array $array1, array $array2 ) {
    $merged = $array1;
    foreach ( $array2 as $key => $value ) {
        if ( is_array( $value ) && isset( $merged[$key] ) && is_array( $merged[$key] ) ) {
            $merged[$key] = redux_array_merge_recursive_distinct ( $merged[$key], $value );
        } else {
            $merged[$key] = $value;
        }
    }

    return $merged;
}
kprovance commented 9 years ago

Generally, anyone with a panel with 1000 or more options is doing it wrong anyway. Does this alter the way data is stored in the database?

harunbasic commented 9 years ago

It doesn't alter how data is stored. parse_str() converts the serialized string into an array of options. This function does exactly the same. It creates exactly the same array as the parse_str() does but without that limit.

kprovance commented 9 years ago

Okay. Let me play with this. I have to be sure such a change works under a few different conditions and doesn't cause breakage with our extensions library. :)

kprovance commented 9 years ago

I spoke to Dovy about this. He says "use section ids, because if you don't, it uses the title in place, and that's why hes having issues." There you go. :)

harunbasic commented 9 years ago

I think you have answered the wrong issue :). Issue with section ids is here https://github.com/reduxframework/redux-framework/issues/2629

This is about ajax save.

kprovance commented 9 years ago

Yeah, whoops. My bad. Still paying with your code. So far, so good. Has to pass the customizer test yet. ;-)

harunbasic commented 9 years ago

Just found out that in the text fields it removes the + sign. Needs a bit tuning. I'm working on a solution.

kprovance commented 9 years ago

Another field or two you might want to also check (which I have not yet, as I'm working on something else), would be the ACE Editor and WP Editor. We've had issues with those fields in the past in terms of ajax save.

kprovance commented 9 years ago

I can confirm that the + sign also disappears in the ACE Editor.

harunbasic commented 9 years ago

Fixed it. In the redux_parse_str() the urldecode() isn't necessary and was the reason why the + was disappearing.

Instead of $pairs = explode( '&', urldecode( $string ) ); there should be $pairs = explode( '&', $string );.

I have tested it with all characters on my keyboard and it works like it should.

kprovance commented 9 years ago

Confirmed. Cool. Okay, still putting this through it's paces. So far so good. :)

anikitas commented 9 years ago

Hello,

I have the same issue too with 1015 variables. I know that i have a large config file.

I usually have max_inputs_vars more than the default 1000, i switched back to the default value 1000 to see how Redux reacts.

Tested also the code and looks that it can save now properly with ajax.

Will you include similar implementation to this in one of the next releases?

Thanks in advance!

anikitas commented 9 years ago

I will test it again,

I think multi select is not saving correctly with this code.

I tested a field:

'type'     => 'select',
'data'     => 'post_type',
'multi'    => true,

It saves only one value not all selected.

anikitas commented 9 years ago

I think the recursive function needs a little modification.

function redux_array_merge_recursive_distinct( array $array1, array $array2 ) {
    $merged = $array1;
    foreach ( $array2 as $key => $value ) {
        if ( is_array( $value ) && isset( $merged[$key] ) && is_array( $merged[$key] ) ) {
            $merged[$key] = redux_array_merge_recursive_distinct ( $merged[$key], $value );
        } else if ( is_numeric( $key ) ) {
            if ( !in_array( $value, $merged ) ) {
                $merged[] = $value;
            }       
        } else {
            $merged[$key] = $value;
        }
    }
    return $merged;
}

You can double check if you want!

kprovance commented 9 years ago

@harunbasic , can you confirm? I admit, I've not had time to do so yet.

harunbasic commented 9 years ago

Yes, multi select and multi text wasn't working. It was saving only the last value.

@anikitas your code fixed multi select but broke multi checkbox with data and other fields that used unique integer for key.

Here is the recursive function that is working for all fields.

function redux_array_merge_recursive_distinct( array $array1, array $array2 ) {
    $merged = $array1;
    foreach ( $array2 as $key => $value ) {
        if ( is_array( $value ) && isset( $merged[$key] ) && is_array( $merged[$key] ) ) {
            $merged[$key] = redux_array_merge_recursive_distinct( $merged[$key], $value );
        } else if ( is_numeric( $key ) && isset( $merged[$key] ) ) {
            $merged[] = $value;  
        } else {
            $merged[$key] = $value;
        }
    }
    return $merged;
}

I have gone through all fields in the sample-config.php and checked it.

anikitas commented 9 years ago

Great! I will test it later on. Thanks.

anikitas commented 9 years ago

I think the last function is the best. It fixes the multi fields, otherwise the result is not the same. I made several tests, comparing the outputs with the sample-config.php, With the last method the outputs are identical.

I also found a notice in the process. Not related with the alternative method of saving the fields.

You can reproduce it in debug mode with the sample config file, if you Reset All or Reset Section Found in the following menu: Basic Fields - Radio - Radio Option w/ Menu Data

Notice: Array to string conversion in C:\wamp\www\blade\wp-includes\general-template.php on line 3261

I made all tests again with: Redux v3.5.8

anikitas commented 9 years ago

On the safe side a developer could add a setting in options to switch between two functions. The developer can decide to use the second method if he has many parameters, so current installations are not affected.

Like the ajax_save parameter e.g: ajax_max_input

if ( isset( $redux->args['ajax_max_input'] ) && $redux->args['ajax_max_input'] == true ) {
    $values = $this->redux_parse_str( $_POST['data'] );
} else {
    parse_str( $_POST['data'], $values );
}

Just a thought!

harunbasic commented 9 years ago

I also think this is a good idea to have that option.

Until this gets implemented you could just create your own save method and overwrite the default one.

The functions are renamed(custom_parse_str) but do the same thing and you will need to replace the custom_options with your opt_name.

Also it would be good to rename the functions that start with custom :)

if ( ! function_exists( 'custom_redux_ajax_save' ) ) {
    /**
     * Custom redux ajax save that has not a limit for max_input_vars
     * because we aren't using the parse_str functions. We are using custom custom_parse_str function.
     *
     * @since   1.0
     * @access  public
     * @return  array $return_array
     */
    function custom_redux_ajax_save() {
        if ( ! wp_verify_nonce( $_REQUEST['nonce'], "redux_ajax_noncecustom_options" ) ) {
            echo json_encode( array(
                'status' => __( 'Invalid security credential.  Please reload the page and try again.', 'custom' ),
                'action' => ''
            ) );

            die();
        }

        if ( ! current_user_can ( 'manage_options' ) ) {
            echo json_encode( array(
                'status' => __( 'Invalid user capability.  Please reload the page and try again.', 'custom' ),
                'action' => ''
            ) );

            die();
        }

        $redux = ReduxFrameworkInstances::get_instance( 'custom_options' );
        $dir = $redux::$_dir;
        if ( ! empty( $_POST['data'] ) ) {
            $values = array();
            $_POST['data'] = stripslashes( $_POST['data'] );
            //parse_str( $_POST['data'], $values );
            $values = custom_parse_str( $_POST['data'] );
            $values = $values['custom_options'];
            if ( function_exists( 'get_magic_quotes_gpc' ) && get_magic_quotes_gpc() ) {
                $values = array_map( 'stripslashes_deep', $values );
            }
            if ( ! empty ( $values ) ) {
                try {
                    if ( isset( $redux->validation_ran ) ) {
                        unset( $redux->validation_ran );
                    }
                    $redux->set_options( $redux->_validate_options( $values ) );
                    $do_reload = false;
                    if ( isset( $redux->reload_fields ) && ! empty( $redux->reload_fields ) ) {
                        if ( ! empty( $redux->transients['changed_values'] ) ) {
                            foreach ( $redux->reload_fields as $idx => $val ) {
                                if ( array_key_exists( $val, $redux->transients['changed_values'] ) ) {
                                    $do_reload = true;
                                }
                            }
                        }
                    }

                    if ( $do_reload || ( isset ( $values['defaults'] ) && ! empty ( $values['defaults'] ) ) || ( isset ( $values['defaults-section'] ) && ! empty ( $values['defaults-section'] ) )) {
                        echo json_encode( array( 'status' => 'success', 'action' => 'reload' ) );
                        die ();
                    }
                    require_once $dir . 'core/enqueue.php';
                    $enqueue = new reduxCoreEnqueue( $redux );
                    $enqueue->get_warnings_and_errors_array();
                    $return_array = array(
                        'status'   => 'success',
                        'options'  => $redux->options,
                        'errors'   => isset ( $redux->localize_data['errors'] ) ? $redux->localize_data['errors'] : null,
                        'warnings' => isset ( $redux->localize_data['warnings'] ) ? $redux->localize_data['warnings'] : null,
                    );
                } catch ( Exception $e ) {
                    $return_array = array( 'status' => $e->getMessage() );
                }
            } else {
                echo json_encode( array( 'status' => __( 'Your panel has no fields. Nothing to save.', 'custom' ) ) );
            }
        }
        if ( isset( $return_array ) ) {
            if ( $return_array['status'] == "success" ) {
                require_once $dir . 'core/panel.php';
                $panel = new reduxCorePanel( $redux );
                ob_start();
                $panel->notification_bar();
                $notification_bar = ob_get_contents();
                ob_end_clean();
                $return_array['notification_bar'] = $notification_bar;
            }
            echo json_encode( apply_filters( "redux/options/custom_options/ajax_save/response", $return_array ) );
        }
        die ();
    }
}
remove_action( 'wp_ajax_custom_options_ajax_save', array( 'ReduxFramework', 'ajax_save' ) );
add_action( 'wp_ajax_custom_options_ajax_save', 'custom_redux_ajax_save' );
anikitas commented 9 years ago

I can wait a week, part of the code is already present in the latest version of Redux.

Function redux_array_merge_recursive_distinct needs to be updated and possibly the check in the ajax_save function.

Of course this is up to Kevin how to implement it.

kprovance commented 9 years ago

Okay, status check. :)

I'm not sure where we are on the most final form of this code. The functions are currently in the core, but their not being run yet, as I thought a problem with button_set prevented it? Am I remembering that correctly?

As it stands now, what the are current (and working) versions of these functions, so I can update the core.

I could add the argument to turn it off, but I have to ask why? What would be a reason to want to turn it on or off?

anikitas commented 9 years ago

Function redux_array_merge_recursive_distinct needs to be updated with this one. It has one more check.

function redux_array_merge_recursive_distinct( array $array1, array $array2 ) {
    $merged = $array1;
    foreach ( $array2 as $key => $value ) {
        if ( is_array( $value ) && isset( $merged[$key] ) && is_array( $merged[$key] ) ) {
            $merged[$key] = redux_array_merge_recursive_distinct( $merged[$key], $value );
        } else if ( is_numeric( $key ) && isset( $merged[$key] ) ) {
            $merged[] = $value;  
        } else {
            $merged[$key] = $value;
        }
    }
    return $merged;
}

The switch could be as a precaution for further testing. Or if we find a issue to be able to switch it back to the old method that works well for < 1000 parameters.

Some fields require more parameters than others ( like Typography ) thus you end up with many params.

As you said not everybody uses more than 1000 parameters in total.

harunbasic commented 9 years ago

I was thinking if you aren't done with the testing and aren't quite sure should you use this save method then you could add that switch so only developers that want this new save method can use it.

But if you are done with the testing and you think that there is no reason to use the old parse_str anymore then the switch isn't necessary.

As I said before I have gone through all fields in the sample-config.php and everything is working like it should.

kprovance commented 8 years ago

@harunbasic -I know it's been a while since I visited this. I was going to patch this back in and give it another run. Maybe WordPress did something since this code, I don't know...but as it stands, something either isn;t saving or being retrieved properly. For example, if one sets an switch option to a new setting, and clicks Save, everything looks kosher. Hit refresh, and the setting reverts. Somewhere between Oct and now, something got wonky.

Are you still using this code? Does it need to be updated?

Thanks!

kprovance commented 8 years ago

I did a little leg work for this:

As it stands, the saves made with this code ARE saving to the database. However, when refreshing the page, the values are not being pulled from the database...everything is the default value. I can only surmise there is something in the save cause Redux to not see the proper values and load the default? Not sure. Don;t have the time to dig into the why of it all, but that seems to be the standing issue.

Cheers.

kprovance commented 8 years ago

Okay, you know what? Never mind any of what I just wrote. I'm a freakin' idiot sometimes. The fault was completely mine, commenting out the wrong bloody line! Ugh! :)

So far so good. I'm thinking we'll push this to our repo and see what happens with a broader audience.

Cheers.

kprovance commented 8 years ago

Adding manually. If new issues crop up, let's start a new thread for it.

designsvilla commented 8 years ago

new version not saving options with ajax_save set to true

kprovance commented 8 years ago

Sorry, but it does. Tens of thousands of people use it daily. However, you're going to need to be WAY more specific. Please review: https://github.com/reduxframework/redux-framework/blob/master/CONTRIBUTING.md

designsvilla commented 8 years ago

i am using redux in my project earlier version was working fine. i have just integrate new version and every thing working fine except options not saving when ajax_save is true and console shows error

There was an error saving. Here is the result of your action
Catchable fatal error: Argument 1 passed to ReduxFramework::redux_array_merge_recursive_distinct() must be of the type array, string given, called in D:\xamp\htdocs\wordpress\helpme\helpme\wp-content\themes\helpme\includes\framework\ReduxCore\framework.php on line 3914 and defined in D:\xamp\htdocs\wordpress\helpme\helpme\wp-content\themes\helpme\includes\framework\ReduxCore\framework.php on line 3935

while if ajax_save is false its working fine

i think max_input_vars delimiter need some work

harunbasic commented 8 years ago

I'm using it and there are no problems. Are you using some custom field or is the error accuring on some default field?

Attach your files (config file and maybe your custom field if there is one) so we can see.

designsvilla commented 8 years ago

these are framework and config files

On Sat, Mar 19, 2016 at 8:15 AM, Harun Bašić notifications@github.com wrote:

I'm using it and there are no problems. Are you using some custom field or is the error accuring on some default field?

Attach your files (config file and maybe your custom field if there is one) so we can see.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/reduxframework/redux-framework/issues/2631#issuecomment-198625792

harunbasic commented 8 years ago

Where are they? I don't see anything.

designsvilla commented 8 years ago

in attachment

On Sat, Mar 19, 2016 at 10:15 PM, Harun Bašić notifications@github.com wrote:

Where are they? I don't see anything.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/reduxframework/redux-framework/issues/2631#issuecomment-198750829

kprovance commented 8 years ago

There are no attachments on the issue tracker, mate. And this is not being done via email. Use Gist to provide the information: https://gist.github.com/

Thanks for stepping back in @harunbasic. :)

designsvilla commented 8 years ago

sorry attachments was sent through mail please find here

ReduxCore.zip

kprovance commented 8 years ago

Okay, you're not understanding what we need here. We don't need a zip of our own code base. I already have that, since I'm one of the devs, right?

What we need is your options config. We have to be able to reproduce your particular issue. We cannot fix what we cannot break.

Thanks. :)

designsvilla commented 8 years ago

option-config is attached in zip which is i am using for