Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

gustavs-gutmanis
Copy link

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

Referencing issue #6

…odel value.

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

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants