search

impress-org / google-maps-builder

One Google Maps plugin to rule them all. Google Maps Builder is intuitive, sleek, powerful and easy to use. Forget the rest, use the best.
https://wordpress.org/plugins/google-maps-builder/
32 stars 9 forks source link

Multiple keys being used to access API key option #211

Closed lchski closed 8 years ago

lchski commented 8 years ago

Discovered this last night with the Pro repo, though it may have impacts elsewhere.

Across the repos, we use different option keys to access the API key option. A semi-comprehensive list:

There are three unique keys: gmb_maps_api_key, maps_api_key, and gmb_api_key. They appear multiple times, all three forms, in the core repo, but in the Free repo only gmb_api_key appears and in Pro only maps_api_key.

This caused trouble for me, because my database only had the most recent version of the plugin installed. My API key is stored in gmb_maps_api_key. When maps_api_key or gmb_api_key were called, they returned false. This caused some odd side effects: the main map builder interface would work, but the mashup metabox wouldn’t (somewhat related to #184 and #176); the banner telling me to add my API key (added already by @DevinWalker, original issue #174) was appearing even though I had my API key set, because it was checking for the existence of the wrong variable.

We need to reconcile all these variables somehow. This could happen via an upgrade process (preferable), or we could rig up something a bit more hacky, extending the gmb_get_option() function to check against all three possible values. Upgrade process definitely preferable, even if a bit more work.

@mathetos, you added me to write up a process to verify the upgrade would works; I can work on that, and I’m happy to hear other thoughts, just wanted to get this issue out first to see whether there’s some simpler solution.

DevinWalker commented 8 years ago

Thanks for the thorough reply here @lchski - let's add to our upgrade routine for 2.1 to consolidate the API key to a single option throughout both plugin versions with backwards compatibility.

lchski commented 8 years ago

I’m going to merge all the keys into the gmb_maps_api_key key, as it’s the most descriptive.

lchski commented 8 years ago

I’ve made progress with this in the core and pro repos. The core upgrade function checks for all three possible API keys, checks for duplicates, picks one and sets it as gmb_maps_api_key.

I’d like some guidance on this next point, though: I’ve updated references to the other API key values throughout the codebase, except for in the 2.0 upgrade functions (core and free). I left the 2.0 upgrade functions alone because they refer to gmb_api_key.

As of the merged codebase refactor, where the settings are managed in the core repo, the key is now gmb_maps_api_key. I wasn’t sure how to proceed here—do we try to run the 2.1 API key upgrade function before the 2.0 upgrade function, then change the 2.0 upgrade function to refer to the new key? Or do we, as I have now, leave it as-is and allow the 2.0 upgrade function to refer to the old key, then update the database to use the new key? The risk in changing the 2.0 upgrade function (which is already production/deployed code) is that it’ll refer to what might be a non-existent key, gmb_maps_api_key. It might not be an actual problem, though, and I’d appreciate a second set of eyes on the situation if possible. I apologize if this is confusing, but it’s important that we get this bit right.

It might be a matter of working up a testing routine, which will show whether or not we’re safe to change the 2.0 upgrade function’s reference or whether we should leave it as-is.

lchski commented 8 years ago

Just created PRs for the two branches. @DevinWalker, I’d love your thoughts on the changes and whether they make sense. I’m particularly interested in the problem of the 2.0 upgrade function—I wonder whether it might be entirely circumvented if we were to run the API key upgrade before any other.

DevinWalker commented 8 years ago

Thanks @lchski - I'll review most likely tomorrow or early next week and get back to you. I read your comment above and it is a bit confusing to understand without looking at the code. After I get a chance to review I may have a follow up question or two.