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

Bumped dependencies and code for php 7.4 compatibility #461

Merged
merged 8 commits into from Mar 6, 2020

Conversation

@Maaiins
Copy link
Contributor

Maaiins commented Feb 29, 2020

ref: #414 #432

Maaiins added 8 commits Feb 29, 2020
@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 6, 2020

I wonder if we can eliminate the use of JSON.php. It was added way back when PHP didn't have built in JSON support. At a minimum i think we can add add some static methods to the owa_lib class such as owa_lib:json_encode and owa_lib::json_decode that abstracts away the need for the rest of the code base to know about JSON.php. Thoughts?

@padams padams merged commit c63b578 into Open-Web-Analytics:master Mar 6, 2020
@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 6, 2020

Great cleanup.

@Maaiins

This comment has been minimized.

Copy link
Contributor Author

Maaiins commented Mar 6, 2020

I think there are a few more things to improve. For example adding dependencies via composer. When doing this the cleanup of old deprecated libraries is also an option.
But I think there is no need to add static methods, you can just use built in functions like json_encode() or the Symfony Serializer component (For more advanced usage). I don't know which one is the right one at the moment.

@Maaiins Maaiins deleted the Maaiins:php7.4 branch Mar 6, 2020
@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 6, 2020

maybe try the built in php method first and let's see if that is compatible output.

@Maaiins

This comment has been minimized.

Copy link
Contributor Author

Maaiins commented Mar 7, 2020

I found out the JSON.php file is used in owa_jsonView and owa_jsonResultsView classes only. I searched for the usage of the classes but they are unused. Can that be correct? If that´s true simply remove the file and both classes.

I´ve done this for me, there was no error in admin or when tracking events.

@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 7, 2020

That is entirely possible. I did notice that the resultSetManager already uses the standard php json_encode method so it's likely that the refactoring started but was never completed.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.