license key check FAIL causes rules to be flushed

[futtta]

cfr. mail earlier today; due to a problem on Jonas' side the license key check temporarily failed while I was testing the "queue cleanup"-improvements. Once the license key check succeeded again, all my existing rules had been flushed, but rules should persist even if the license key check fails.

[denydias]

I'll change the cleanup tasks from register_deactivation_hook to register_uninstall_hook.

This will bring two downsides:

1. The deactivation/activation dance to upgrade behaviors will not work anymore. Users will have to uninstall/install the plugin using wp-admin so the new behaviors works as expected.

2. Users should not uninstall the plugin by deleting its directory. If a user just delete the plugin directory, register_uninstall_hook will not be fired and there will be orphaned db options left behind.

Is that acceptable?

Should I fix this in the major 1.0.0 milestone or can I close it and open a new minor one? All its issues are closed now, that's why I'm asking.

[futtta]

dumb question maybe, but why would a license check fail trigger actions hooked into plugin deactivation (or uninstallation)?

On Thu, Apr 19, 2018 at 8:29 AM, Deny Dias notifications@github.com wrote:

I'll change the cleanup tasks from register_deactivation_hook to register_uninstall_hook.

This will bring two downsides:

1.

The deactivation/activation dance to upgrade behaviors will not work anymore. Users will have to uninstall/install the plugin using wp-admin so the new behaviors works as expected. 2.

Users should not uninstall the plugin by deleting its directory. If a user just delete the plugin directory, register_uninstall_hook will not be fired and there will be orphaned db options left behind.

Is that acceptable?

Should I fix this in the major 1.0.0 milestone or can I close it and open a new minor one? All its issues are closed now, that's why I'm asking.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/28#issuecomment-382625363, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMaqbTSS8dgZhXM927eICF7N_oAxrks5tqC7mgaJpZM4TYGZg .

[denydias]

No questions are dumb. Answers maybe... just like that: because I did it that way as there was a lack of specs for this feature. :P

When the license check fail, it's just like a new beginning.

[futtta]

so can we undo that link? so if license check fails, just stop access to the admin functionality (except for the license key fiels), but leave all else as is?

On Thu, Apr 19, 2018 at 8:46 AM, Deny Dias notifications@github.com wrote:

No questions are dumb. Answers maybe... just like that: because I did it that way as there was a lack of specs for this feature. :P

When the license check fail, it's just like a new beginning.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/28#issuecomment-382628604, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMbUsnz4r7C0QP2d5I27xAP-k_8gpks5tqDKugaJpZM4TYGZg .

[denydias]

Sure! In the current milestone or the next one?

[futtta]

You can do it in this one :)

On Thu, Apr 19, 2018 at 9:02 AM, Deny Dias notifications@github.com wrote:

Sure! In the current milestone or the next one?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/28#issuecomment-382632175, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMbA5g61Klyq0KtZARWeJFPpCn-63ks5tqDaZgaJpZM4TYGZg .

[denydias]

Ok then. I'm a bit busy with other stuff in the next two days. In the weekend I'm going to ride my bike for camping nearby a waterfall. Get back to it on monday, ok?

[futtta]

Sure, no issues mate, enjoy the bike trip!! :-)

On Thu, Apr 19, 2018 at 9:19 AM, Deny Dias notifications@github.com wrote:

Ok then. I'm a bit busy with other stuff in the next two days. In the weekend I'm going to ride my bike for camping nearb waterfall. Get back to it on monday, ok?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/28#issuecomment-382635978, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMeCGY-b8wzjmkYUiHPjXZMfPAufVks5tqDp8gaJpZM4TYGZg .

[denydias]

Oh yeah, I'll! :motor_scooter::dash::dash:

Thanks!

[denydias]

Frank, I'm looking into this right now. It turns out this is not a bug. What happens is described bellow:

1. When a key validation fail, none of the plugin's wp_options settings are touched (incl. rules and queue). They remain just the same. You can check this by enabling debug, entering a invalid key and saving the changes. All the autoptimize_ccss_* are going to be kept.

2. Now that you have an invalid key, all the other panels should be hidden. As such, the DOM properties of all fields but the key one doesn't exist. That includes their values.

3. If you correct the key and save the form again, the form have a value for the key but there'll be no other values to save. Hence they are saved as empty. At this point this is going to reset the settings (incl. rules and queue).

By following the stated above, if the cron's ao_ccss_api_generate() and ao_ccss_api_results() set the key as invalid after a ccss.com service outage and in the next run it validates fine again, all settings remains as they were. The settings are reset upon user interaction with the settings page only.

The thing is that this behavior comes since the earlier stages of the plugin development, when we decided the order of the panels (the key panel was moved to last) and to hide other panels until key activation. To change that behavior I need to implement a way to populate all the plugin settings from the wp_options as hidden fields for the ones in hidden panels if the key is invalid and there are values for them in the DB. This is going to be quite an effort and by all means this is a new feature, not a bugfix.

What should I do here? Send you an effort or leave it as is?

[futtta]

• a relief settings remains as they were when there's no user interaction
• the fact that any key-change or accidental re-submission of the key (as I did) removes all rules remains problematic
• my old (non-automated) plugin indeed had the rules-object as a hidden field for this very reason;

    <form method="post" action="options.php">
        <?php settings_fields( 'critcss-settings-group' ); ?>
        <input type="text" id="critLicense" name="critLicense" style="width:100%;" placeholder="<?php _e("Please enter your license key.","ao_critcss"); ?>" value='<?php echo get_option("critLicense",""); ?>'>
        <input type="hidden" id="critCssOrigin" name="critSettings" style="display:none;" value='<?php echo (json_encode ((object)$critSettings)); ?>'>

        <p class="submit">
            <input type="submit" class="button-primary" value="<?php _e('Save Changes','ao_critcss') ?>" />
        </p>
    </form>

And that is (all) we need really; to persist the rules. Doesn't have to be that complicated, does it? :-p

[denydias]

We can't just throw out hidden fields at will and expect that them to just work as magic. You have to remember that we are hiding panels for a reason (unclutter UI before it becomes useful). So, if we just add those hidden fields, they MUST be present only when there are no other fields with same names to avoid saving outdated values into DB, as JS handles everything under the hood expecting values from form fields.

As such, unfortunately yes, it is that complicated. Your old plugin doesn't come with all those fancy UI we have now, that's why you have this feeling that this is not a feature and it's not complicated.

I insist this is not a bug in the way we've designed so far. To handle that we need a new feature, which is precisely "add hidden fields filled with values from DB only if there's no active key." New features comes with an effort. Bugfixes are free of charge.

[denydias]

Frank, implemented free of charge! :wink:

Ready for testing.

[futtta]

tested, works perfectly! thanks @denydias ! :-)

[denydias]

Can I close the milestone and tag the release?

[futtta]

yes, you can Sir! :-)

On Fri, Apr 27, 2018 at 4:26 AM, Deny Dias notifications@github.com wrote:

Can I close the milestone and tag the release?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/28#issuecomment-384844851, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMXTOLLmYT5c6qoGImpnekmlFnVzNks5tsoHQgaJpZM4TYGZg .