search

creof / doctrine2-spatial

Doctrine2 multi-platform support for spatial types and functions.
MIT License
272 stars 176 forks source link

Version 2? #209

Open Alexandre-T opened 4 years ago

Alexandre-T commented 4 years ago

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

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-22.2%) to 75.822% when pulling bbb39cd00024d997f47df503341f402e554afec2 on Alexandre-T:merge-creof into 58ea5fae1c1b450ee08d7dac25cd9e8f5e6fdebd on creof:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-4.08%) to 93.952% when pulling 0d4c1af08ea5c6d2e6b72a005aad7cd7b069e0b1 on Alexandre-T:merge-creof into 58ea5fae1c1b450ee08d7dac25cd9e8f5e6fdebd on creof:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-4.08%) to 93.952% when pulling 0d4c1af08ea5c6d2e6b72a005aad7cd7b069e0b1 on Alexandre-T:merge-creof into 58ea5fae1c1b450ee08d7dac25cd9e8f5e6fdebd on creof:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-4.08%) to 93.952% when pulling 0d4c1af08ea5c6d2e6b72a005aad7cd7b069e0b1 on Alexandre-T:merge-creof into 58ea5fae1c1b450ee08d7dac25cd9e8f5e6fdebd on creof:master.

sadortun commented 4 years ago

Wow, awesome work !

djlambert commented 4 years ago

@sadortun agreed!

I haven't had time to contribute for quite a while, and haven't been working with PHP much the last couple years. @sadortun maybe you're in a better position to merge this? It looks like coveralls is being picky. I have 4k monitors and could care less about line lengths.

cedvan commented 4 years ago

:+1:

cedvan commented 4 years ago

Hi,

I find a bug, postgresql function ST_Distance_Sphere was renamed in postgresql by ST_DistanceSphere. So fix is necessary in CrEOF\Spatial\ORM\Query\AST\Functions\PostgreSql\SpDistanceSphere

See you

Alexandre-T commented 4 years ago

I will edit it. But, this bug is surprising, because this function is tested in this functional test. My test should failed and it didn't! Why?

edit: Oh, ok! The function was renamed on postgis!

Alexandre-T commented 4 years ago

@sadortun , do you think you can merge this as suggested by @djlambert ? If you can't, how can I help you?

sadortun commented 4 years ago

@Alexandre-T can you fix the merge conflicts ?

Thanks

tugrul commented 4 years ago

Please don't change protected fields to private. This could be break existing codebase of projects. protected is better to extend library code without touch library's source. private is logical for rare cases like keep plaintext password before hash because we don't want leak the information possible flaw or if some classes has many execution steps and if there a possible break when we touch a member of the class then we can declare as private. But we should not use always private because it kills flexibility.

edit: also you can consider same thing for final decleration

Alexandre-T commented 4 years ago

@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 commented 4 years ago

@Alexandre-T Thanks for this PR

Alexandre-T commented 4 years ago

@sadortun Is there anything else blocking the merge?

sadortun commented 4 years ago

@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.

1) 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 :

2) Will this cause issues with other @creof libraries ?

3) 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.

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

Alexandre-T commented 4 years ago

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.

rogeriolino commented 3 years ago

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

rogeriolino commented 3 years ago

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

PowerKiKi commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 https://github.com/creof/doctrine2-spatial/pull/209#issuecomment-653941286

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

samwilson commented 3 years ago

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 commented 3 years ago

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.