owncloud-archive / maps

:globe_with_meridians: Maps app for ownCloud
GNU Affero General Public License v3.0
42 stars 20 forks source link

Refactor Database Mappers #66

Closed Henni closed 8 years ago

Henni commented 9 years ago

I refactored the database mappers. This PR replaces the current implementation with entities for devices and locations and their corresponding mappers.

Henni commented 9 years ago

At the moment the implementation is broken, but I have no idea why. Somehow the dependency injection doesn't work anymore. Ideas on how to fix this are welcome! image

v1r0x commented 9 years ago

From looking at the changes I see no problem, but I'm no expert in DI ;) Will have a look at it as soon as I have some free time

jancborchardt commented 9 years ago

@DJaeger @brantje @poVoq can you also have a look here?

DJaeger commented 9 years ago

I will have a look as soon as I have some time.

jancborchardt commented 8 years ago

@Henni is this pull request still work in progress or done from your point of view?

Henni commented 8 years ago

We just have to find out why the error occurs. Everything else is done.

Maybe someone who knows a bit about the internals of dependency injection can help.

LEDfan commented 8 years ago

Hey, never checked out this app but saw this issue on IRC. I have solved this error in the commit I made, I hope this isn't a problem?

Some general tips on DI and autoloading:

For reference the documentation of ownCloud: https://doc.owncloud.org/server/8.2/developer_manual/app/container.html and of Pimple (the DI injection container oC use): http://pimple.sensiolabs.org/

Henni commented 8 years ago

@LEDfan thanks for the help! So I'll replace the filenames with lowercase filenames.

Can you explain to me why the error message said something about the cachemanager while it was obviously a problem with the location manager?

LEDfan commented 8 years ago

I guess this is because the changes I made here: https://github.com/owncloud/maps/commit/34979551480ce0a40e510ccb67884e622d65719a#diff-a8d3ad1f02ffca9503fa85aff518fc1dR25 . The DI container knows now which class you need and will load it automatically. If you do this you don't need to register the CacheManager service actually. But you have to when you want to inject special things inside the CacheManager. Note that if you won't want to register the services all classes must be type casted. (line https://github.com/owncloud/maps/commit/34979551480ce0a40e510ccb67884e622d65719a#diff-ae7c5d396a800dcab2fb6f2fc7455d37R19 )

@BernhardPosselt anything to add? (You wrote the code)

LEDfan commented 8 years ago

@Henni is the fix working for you?

Henni commented 8 years ago

@LEDfan Yes the fix is working for me. Again thanks for the help!

BernhardPosselt commented 8 years ago

IDb is deprecated, use IDBConnection ;)

Henni commented 8 years ago

The pull request is now almost complete. I just want to remove the last occurence of the locationmanager.

Therefore I still have to figure out how to return arrays of Entities as a JSON Response.

Henni commented 8 years ago

@BernhardPosselt thanks for the hint. We'll have to refactor the CacheManager as well and replace it with a CacheMapper which uses IDBConnection instead.

Henni commented 8 years ago

Ready to review! The CacheManager will be refactored in a seperate PR (Issue #72).

cc @v1r0x @DJaeger @brantje @poVoq

jancborchardt commented 8 years ago

@v1r0x @DJaeger @brantje @poVoq can you review this? :)

v1r0x commented 8 years ago

I'm back from my holiday and can hopefully review this in the next days

Henni commented 8 years ago

@DJaeger do you think we can merge this. This would simplify future development.

DJaeger commented 8 years ago

I will review this later day. I'm still working thought my pending mails...

Henni commented 8 years ago

@irgendwie

irgendwie commented 8 years ago

LGTM :+1: