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

Added tests for Doctrine Types #6

Merged
merged 7 commits into from
Jan 23, 2015
Merged

Added tests for Doctrine Types #6

merged 7 commits into from
Jan 23, 2015

Conversation

cevou
Copy link
Contributor

@cevou cevou commented Jan 21, 2015

This adds integration tests for the doctrine types. I had to adjust some code, so everything works with PostgreSQL. The tests uses the DbalFunctionalTestCase as base, which is not part of the doctrine archive. Thats why I changed the composer install to prefer sources.

@cevou cevou mentioned this pull request Jan 21, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-14.87%) when pulling 38cecde on cevou:type_tests into ebba501 on brick:master.

Type::addType('multipoint', 'Brick\Geo\Doctrine\Types\MultiPointType');
Type::addType('multipolygon', 'Brick\Geo\Doctrine\Types\MultiPolygonType');
Type::addType('point', 'Brick\Geo\Doctrine\Types\PointType');
Type::addType('polygon', 'Brick\Geo\Doctrine\Types\PolygonType');
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use PHP 5.5's ::class syntax here: use Brick\Geo\Doctrine\Types; at the top of the file, then addType('...', Types\GeometryType::class);.

@BenMorel
Copy link
Member

Thanks for the PR!

I had to adjust some code, so everything works with PostgreSQL.

If I understand correctly from your code, PostGIS has a single GEOMETRY column type, and no sub-types such as POINT and POLYGON?

@cevou
Copy link
Contributor Author

cevou commented Jan 21, 2015

There is a geography type (http://postgis.net/docs/manual-2.1/using_postgis_dbmanagement.html#PostGIS_Geography) with which you can create more specific fields like location GEOGRAPHY(POINT,SRID). Should we use this type? It only supports POINT, LINESTRING, POLYGON, MULTIPOINT, MULTILINESTRING, MULTIPOLYGON and GEOMETRYCOLLECTION.

@BenMorel
Copy link
Member

I'm not sure we should venture into the territory of Geometry vs. Geography for now: there are other implications AFAIK with Geography, related to how distances are calculated etc.
If Geometry does not have sub-types, let's use this one for now?

@cevou
Copy link
Contributor Author

cevou commented Jan 21, 2015

Thats's OK for me.

@coveralls
Copy link

Coverage Status

Coverage decreased (-14.87%) when pulling c08e9c0 on cevou:type_tests into ebba501 on brick:master.

- if [[ $ENGINE = GEOS ]]; then bash .travis.install-geos.sh; fi;
- if [[ $ENGINE = SQLite3 ]]; then bash .travis.install-sqlite3.sh; fi;
- composer install
- composer install --prefer-source
Copy link
Member

Choose a reason for hiding this comment

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

Could you use preferred-install in composer.json instead? So that no-one is confused by missing classes when cloning the repo and running a bare composer install.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to specify process-timeout as well: on my machine, cloning Doctrine repos exceeded the default 300 seconds timeout, so Composer reverted to dist install. Setting process-timeout to 600 worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set the prefer source within the composer.json the packages will always be iinstalled from source. Put this is only required if you want to run the tests. In productive usage it is better to have them loaded from archive. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The doc says that these settings only apply to projects, so it should only apply when you run composer install from within the brick\geo repository (that is, when you're developing the library) not when it's included as a dependency of a third-party project!

@coveralls
Copy link

Coverage Status

Coverage decreased (-9.53%) to 57.35% when pulling 658dffb on cevou:type_tests into ebba501 on brick:master.

@BenMorel
Copy link
Member

Looks good. Thanks!

BenMorel added a commit that referenced this pull request Jan 23, 2015
Added tests for Doctrine Types
@BenMorel BenMorel merged commit 964e3be into brick:master Jan 23, 2015
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.

3 participants