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

Add Geometry and Custom SRID #76

Merged
merged 15 commits into from
Jul 15, 2017
Merged

Add Geometry and Custom SRID #76

merged 15 commits into from
Jul 15, 2017

Conversation

v1r0x
Copy link
Contributor

@v1r0x v1r0x commented May 23, 2017

This PR is based on #35

Additionally I fixed a broken test and added a new exception (if an unsupported geomtype or illegal srid is given). Based on @phaza s comments I tried to fix the models, but could not figure out what exactly needs to be fixed.
To check what's wrong I created a model (Location) based on the example and tried to set it's column location3 (which is type geometry) to $location->location3 = new Point(37.422009, -122.084047);. But this throws an error, because in the PostgisTrait.php file it uses ST_GeogFromText if the following check is true if ($value instanceof GeometryInterface && ! $value instanceof GeometryCollection). My idea was to also check the geomtype parameter which is added in the Blueprint.php. Unfortunately I couldn't find a method to perform this check. Could anyone confirm that this is the correct idea or help me what to do ( @phaza @njbarrett ) ? Thanks!

@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Changes Unknown when pulling 2e8374f on v1r0x:master into ** on njbarrett:master**.

@phaza
Copy link
Collaborator

phaza commented May 23, 2017

It's honestly been so long since I worked on this project I can't remember the intricacies, but I think you pretty much found the current show stopper.

You need to figure out how you can tell the type of a field: Geometry or Geography.
The only two ways I can think of are 1) Ask the database, 2) Make the user (coder) tell you.

My suggestion is probably the latter. The user has to specify $postgisFields on all models anyway, perhaps telling them to specify whether a field is a Geometry or Geography could be a good idea?

@njbarrett should probably weigh in here since he owns the project now.
It should be possible to do this without breaking backwards compatibility by saying that a normal $postgisFields array defaults to geographies, while a keyed one (map, dictionary. associative array) will need specification of type.

@v1r0x
Copy link
Contributor Author

v1r0x commented May 23, 2017

After some more testing I agree with you. I tried to get the datatype from the DB, but due to this line $this->getDoctrineSchemaManager()->getDatabasePlatform()->registerDoctrineTypeMapping('geography', 'string'); in PostgisConnection.php it returns string as datatype. Thus I also think the best solution would be assoc array for $postgisFields.

@v1r0x
Copy link
Contributor Author

v1r0x commented May 23, 2017

I updated the PR using option 2) from your comment @phaza. Was rather easy to do, but one thing which should be considered before making a decision:

  • The $postgisFields is now an assoc array with the column name as key and another assoc array as value (with geomtype and srid). In older versions $postgisFields was a assoc array as well. Don't know if this leads to problems regarding backwards compatibility.

@v1r0x
Copy link
Contributor Author

v1r0x commented Jun 22, 2017

ping @phaza and @njbarrett

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 12, 2017

Is this ok to merge or any further update required? @njbarrett

@njbarrett
Copy link
Collaborator

Hi @v1r0x sorry for delay in responding to this PR.
My issue with merging it currently would be on backwards compatibility with the $postgisFields array becoming an assoc array. If this is merged we may release it as a new major version with a breaking change. However I want to see whether maintaining backwards compat is possible even.
I assume you can use your own fork for now.

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 13, 2017

Thanks for the response @njbarrett
Understandable. I think of another solution without breaking backwards compatibility. It's definitely better to have it in one place instead of a fork(s).

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 13, 2017

One possible solution:
Keep $postgisFields as it is in current master

$postgisFields = ['geom', 'whatever', ...];

and add an additional array:

$postgisTypes = [
    'geom' => [
        'type' => 'geography',
        'srid' => -1 // may be ignored for geography
    ],
    'whatever' => [
        'type' => 'geography',
        'srid' => 1337
    ]
]

If array $postgisTypes exists and the key (get from $postgisFields) exists, it takes the values stored in $postgisTypes. Otherwise it defaults to 'geography'. This should keep backwards compatibilty for older versions with assoc array and current version. What do you think @njbarrett ? If you think it is fine, I would update my PR.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2e7b85e on v1r0x:master into ** on njbarrett:master**.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 13, 2017

Coverage Status

Changes Unknown when pulling 2e7b85e on v1r0x:master into ** on njbarrett:master**.

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 13, 2017

I pushed my idea from above. Should keep backwards compat.
Travis fails due to unsupported combination of hhvm and ubuntu (HHVM is no longer supported on Ubuntu Precise. Please consider using Trusty with dist: trusty`

@njbarrett
Copy link
Collaborator

@v1r0x Thanks. I think I've fixed the builds now. Could you please update your changes to the README as well explaining the new procedure

@coveralls
Copy link

coveralls commented Jul 14, 2017

Coverage Status

Coverage decreased (-2.1%) to 79.321% when pulling 6b0bce6 on v1r0x:master into 4e531f2 on njbarrett:master.

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 14, 2017

@njbarrett Thanks for fixing the builds. I adjusted my changes in the README. Is this ok?

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 79.012% when pulling 525efc6 on v1r0x:master into 4e531f2 on njbarrett:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 79.012% when pulling 525efc6 on v1r0x:master into 4e531f2 on njbarrett:master.

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 14, 2017

I'm going to fix the coverage in another comnit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 80.925% when pulling 3545a4c on v1r0x:master into 4e531f2 on njbarrett:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 80.925% when pulling 3545a4c on v1r0x:master into 4e531f2 on njbarrett:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 80.925% when pulling 3545a4c on v1r0x:master into 4e531f2 on njbarrett:master.

@coveralls
Copy link

coveralls commented Jul 14, 2017

Coverage Status

Coverage increased (+2.4%) to 83.815% when pulling b28c742 on v1r0x:master into 4e531f2 on njbarrett:master.

@coveralls
Copy link

coveralls commented Jul 14, 2017

Coverage Status

Coverage increased (+2.4%) to 83.815% when pulling 3989437 on v1r0x:master into 4e531f2 on njbarrett:master.

@njbarrett
Copy link
Collaborator

njbarrett commented Jul 15, 2017

Thanks @v1r0x ! Lets do this. Thanks for all your hard work.

@njbarrett njbarrett merged commit 53e0d18 into mstaack:master Jul 15, 2017
@njbarrett
Copy link
Collaborator

Released tag 3.2 with this

@v1r0x
Copy link
Contributor Author

v1r0x commented Jul 15, 2017

You're welcome. Thanks for the support and merging it.

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