search

cyberhobo / wordpress-geo-mashup

Official repository for Geo Mashup, the plugin that makes WordPress into a GeoCMS. Documentation:
https://github.com/cyberhobo/wordpress-geo-mashup/wiki/Getting-Started
63 stars 15 forks source link

Notice: Undefined index: geo_mashup_null_fields in geo-mashup-ui-managers.php #823

Closed jerclarke closed 5 years ago

jerclarke commented 5 years ago

Not sure if this is something small or something big. I finally did an update after about two years and after saving a post I got this error:

Notice: Undefined index: geo_mashup_null_fields in /[...]/wp-content/plugins/geo-mashup/geo-mashup-ui-managers.php on line 308

The actual error is very natural, because the code in save_posted_object_location() does a lot of assuming that $_POST will contain things:

$post_location['admin_code']     = sanitize_text_field( $_POST['geo_mashup_admin_code'] );
$post_location['sub_admin_code'] = sanitize_text_field( $_POST['geo_mashup_sub_admin_code'] );
$post_location['locality_name']  = sanitize_text_field( $_POST['geo_mashup_locality_name'] );
$post_location['set_null']       = $this->sanitize_null_fields( $_POST['geo_mashup_null_fields'] );

The last one of course causes this particular error.

So the most direct solution, the one I'm implementing in my copy for now, is to validate teh $_POST field before accessing it, which addresses the notice itself. Really, we should always be doing this any time we access $_POST or $_GET, it's annoying but a good practice because notices like this can really blow up a system that's configured with tight error handling:

if (!isset( $_POST['geo_mashup_null_fields']))
    $post_location['set_null'] = '';
else
    $post_location['set_null']       = $this->sanitize_null_fields( $_POST['geo_mashup_null_fields'] );

Or something along those lines.

The bigger question is why geo_mashup_null_fields is undefined for me, and if that is likely to cause problems.

I can't seem to find any references to "null fields" in the settings.

Thanks for any help you can provide and for figuring out how to make this notice go away for the next version.

cyberhobo commented 5 years ago

Thanks for the detailed report. My quick diagnosis is that this is just a bug that hasn't been visible enough to catch my attention, and should be easy to fix.

I think geo_mashup_null_fields only has a value when something needs to be erased, the saved_name field in this case. Most of the time it is missing and causes the notice, but nothing is wrong.

As a historical note, the reason the whole null fields thing exists has to do with trac ticket #15158. Since it's been around since 2005, Geo Mashup has traces of quite a few WordPress legacies inside.

As a practical note, running Geo Mashup on a system that will blow up on a PHP notice is risky! It's been an open source hobby project from the beginning and has been useful for many people, but has never achieved the resources that would be needed to get the probability of notices down near zero.

jerclarke commented 5 years ago

Thanks Dylan, I hear you about the legacy hacks and the difficulties of reducing all notices.

Of course our production system doesn't blow up on notices, but even when developing, something like this that shows a notice when saving a post can get in the way.

Thanks for figuring out a fix <3