doublesecretagency / craft-googlemaps

Google Maps plugin for Craft CMS - Maps in minutes. Powered by the Google Maps API.
https://plugins.doublesecretagency.com/google-maps/
Other
10 stars 8 forks source link

Fixed a fatal error that happens when trying to normalize an AddressM… #14

Closed gustavs-gutmanis closed 3 years ago

gustavs-gutmanis commented 3 years ago

This happens when Solspace Calendar is serializing an event to JSON and all the custom field values are being normalized.

Referencing issue #6

lindseydiloreto commented 3 years ago

Thanks @gustavs-gutmanis! It looks like this works (doesn't break anything). I may need to tweak it slightly once I understand it better.

Would you mind explaining a little more about what exactly this patch is doing, and why it is necessary? I don't like adding code that I don't fully understand, I'm sure you understand. 🙂

lindseydiloreto commented 3 years ago

I saw your comment on the related thread...

... this issue is happening because Solspace Calendar is serializing all its values when a JSON representation is requested, this means that it's going through all its custom fields and is calling the normalizeValue(), which happens to be an doublesecretagency\googlemaps\models\Address instance.

... but would still appreciate some additional clarification. Thanks!

gustavs-gutmanis commented 3 years ago

The issue that's happening stems from your AddressFields ::normalizeValue() method. At the very end of it, there's a check for $values presence. If it's present, it gets converted to float and added to the distance attribute. However, when Calendar has to be displayed in the frontend via a JS UI, the fetched events get converted to JSON. During that conversion, all of the custom fields are being cast to a JSON compatible value as well, thus the AddressField::normalizeValue() method is invoked, only this time the $value is an instance of AddressModel instead of some other value, you would know more about that. All I did was add a check to see if the $value is an instance of AddressModel and if it is, I set the $value to be the AddressModel's distance property, since the value is later used for setting the distance attribute. Again, you would know more about the validity of this code, perhaps if you get the AddressModel the $value should be converted to null.

lindseydiloreto commented 3 years ago

Fantastic, thanks @gustavs-gutmanis! That largely clears things up.

So when we've got a $value that is an AddressModel, it sounds like we need to convert it into a non-model equivalent. I can see why you defaulted back to the distance value, since that is being handled during a proximity search lookup. But your point about just setting it to null is interesting too. A third option, we could use the string-ified version of an Address Model (which would look like "123 Main St, Springfield, CO 44444, United States").

Then I guess the question becomes, what does Calendar want it to be? What is the JSON-ified data being used for? Would it make sense for us to be using the string-ified Address in this situation?

lindseydiloreto commented 3 years ago

After doing a bit more research, I believe my previous comment was incorrect.

I think I've traced it back to a bug in my code. Under these conditions, we are accidentally trying to convert an Address Model to a float. I did not realize that $value could already exist as an Address Model... in that case, we should be returning it immediately as-is.

Instead of the snippet you submitted, I just tried this modification...

// If the value is already an Address model, return it immediately
if ($value instanceof AddressModel) {
    return $value;
}

And that seemed to work. You can even move those lines up to the top of the normalizeValue method as the first check.

I've submitted this fix to Daryl for him to test. If that patch works for him, then I'll push out a release with that fix. 👍

lindseydiloreto commented 3 years ago

Sorry for the delay following up... Daryl has confirmed that my fix works as intended. 👍

I've pushed the fix live in v4.0.3, please update to the latest version when you get a chance.