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

Version 2? #209

Open
wants to merge 151 commits into
base: master
Choose a base branch
from
Open

Version 2? #209

wants to merge 151 commits into from

Conversation

Alexandre-T
Copy link

@Alexandre-T Alexandre-T commented Apr 1, 2020

Hi,

I needed a new version of doctrine2-spatial which is compatible with PHP 7.2+. So I forked your excellent library and updated it. Because of a lot of changes between PHP 5.1 and PHP 7, my updates do NOT warranty backward compatibility. So, if you're agree to merge my pull-request in your repository, you're welcomed, but do not forget that you should create a new major version.

All new features are described in the Changelog file. There is a new documentation. Please, forgive me, I'm not fluent in english.

If you merge it, I will update the documentation to replace references to my package alexandret/doctrine2-spatial by yours.

You can find the travis, code climat and code coverage results here :

Build Status
Code Climate
Coverage Status
Documentation Status

Alexandre

@Alexandre-T
Copy link
Author

Alexandre-T commented Jun 5, 2020

@tugrul this commit removes final keywords and updates private properties to protected.
@cedvan this commit replaces the old postgis function name by the new one (ST_DistanceSphere). I created another class for the deprecated function (ST_Distance_Sphere)
@sadortun I've removed references to my fork alexandret/doctrine2-spatial as you can see in this version of the documentation. When this PR will be merged, I will update URL of the repository on the readthedocs.org website. I can't do it actually, I guess it's because the creof/doctrine2-spatial repository has no directory containing documentation source, yet.

This travis build is corresponding to the last commit. As you can see, all build jobs are "green".

@florida63
Copy link

@Alexandre-T
Thanks for this PR

@Alexandre-T
Copy link
Author

@sadortun Is there anything else blocking the merge?

@sadortun
Copy link
Member

sadortun commented Jul 5, 2020

@djlambert @Alexandre-T I really appreciate the time and effort you put in, and we will merge this soon!.

@djlambert Just to make sure we dont cause a major panic out there, I want your opnion on this.

Since this PR might include BC breaks. it should be bumped to a next major version tag, that prety obvious. But i can see an issue where folks are using dev-master will get BC break. Should we still merge this into master ?

Here the steps i propose :

  • Tag current master into 1.3.0
  • Create branch 1.x based on current master
  • Merge this PR.
  • Tag the updated master into 2.0,0

Will this cause issues with other @creof libraries ?

We probably need a quick note in Readme to explain that "If you are using dev-master and got BC issue after a composer update that you should tag your dependency on ~1.2, or follow some instructions to help to get through the update.

@Alexandre-T We probably need, if not already done, update the CHANGELOG in this PR

public function getSrid()
{
return $this->srid;
return json_encode($json);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return json_encode($this); same output because class implements JsonSerializable

Copy link
Author

@Alexandre-T Alexandre-T Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added JsonSerializable because I read #194 , but (I'm not sure) I encountered an error in a specific case. Do you agree to wait this PR to be merged, then I will try it another time.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexandre-T JsonSerializable is good. I just mention that no need to create array to generate json from toJson function.

following function will be enough because JsonSerializable feature in use.

public function toJson()
{
    return json_encode($this);
}

@Alexandre-T
Copy link
Author

I do agree with your four steps. What do you think about creating a 1.3.x-dev tag instead of a 1.x-dev?

Changelog is already done. I could update the readme to add a small paragraph to avoid panic for developers using dev-master.

### MariaDB 10
This version is **NOT** compatible with MariaDB version. Some spatial functions seems to work but their results are
different from MySQL version (StContains function is a good example). You can contribute, but I suggest to avoid
MySql and MariaDb servers, because of [their shortcomings and vulnerabilities](https://sqlpro.developpez.com/tutoriel/dangers-mysql-mariadb/).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link to the article should be removed. Because it is a subjective opinion about RDBMS choice (expressed in a rather aggressive tone) and has no reason to be mentioned here. It also happens to be in French which most readers might not be able to read. And finally the way it is introduced make it looks like it is about MariaDB only, but it actually is about both MySQL and MariaDB.

As a matter of fact, the whole "Compatibility" section here is a bit confusing. Topics are mixed between "Compatibility between this lib and a RDBMS" and "Compatibility across RDBMS". The fact that StContains returns different result between MySQL and MariaDB does not make this lib incompatible with MariaDB.

@rogeriolino
Copy link

Any news about this fix or some fork that fix it? I'm getting the same error after upgrating to PHP 8.

@rogeriolino
Copy link

nevermind, I see that @Alexandre-T created a new one. Thank you!

@PowerKiKi
Copy link
Contributor

For the records, @rogeriolino was referring to the fork of @Alexandre-T that is available there: https://github.com/Alexandre-T/doctrine2-spatial

@Alexandre-T
Copy link
Author

Alexandre-T commented Jun 18, 2021

I created an organization named LongitudeOne and a new package that will replace and maintain this awesome spatial extension. All improvements from this PR are already included. I published a pre-released which is compatible with PHP8, MySQL5.7, MySQL8, PostgreSQL13 and PostGis 3.1.

@sadortun sadortun reopened this Jun 18, 2021
@sadortun
Copy link
Member

@Alexandre-T we have a lot of people using this extension, if you want to volunteer as a maintainer, we can simply add you here so everybody can use your updates

@sadortun
Copy link
Member

@Alexandre-T

I just realize you've been working on this for the past year and I really appreciate your contributions.

Let me do what I mentioned #209 (comment)

And I'll be ready to merge this PR as. 2.x-beta

@samwilson
Copy link

Is there an idea of when a new version of this package might be released. I've been (happily) using alexandret/doctrine2-spatial, but it's now marked as abandoned.

Thanks for working on all this! It's super useful.

@Alexandre-T
Copy link
Author

Alexandre-T commented Aug 22, 2021

I marked it abandoned, because I replaced it by this one: https://github.com/longitude-one/doctrine-spatial
I didn't want that this library depends on myself, but an organisation.
Yhis library is now compatible with PHP8, all versions of Postgis, PostgreSQL and MySQL and a lot of spatial functions have been added. It is fully documented.

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.

10 participants