Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Refactor Database Mappers #66

Merged
merged 1 commit into from
Oct 21, 2015
Merged

Refactor Database Mappers #66

merged 1 commit into from
Oct 21, 2015

Conversation

Henni
Copy link
Contributor

@Henni Henni commented Aug 14, 2015

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

@Henni
Copy link
Contributor Author

Henni commented Aug 14, 2015

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
Copy link
Contributor

v1r0x commented Aug 16, 2015

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
Copy link
Contributor

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

@DJaeger
Copy link
Contributor

DJaeger commented Aug 20, 2015

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

@jancborchardt
Copy link
Contributor

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

@Henni
Copy link
Contributor Author

Henni commented Sep 4, 2015

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
Copy link

LEDfan commented Sep 4, 2015

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:

  • use all lowercase for directorie and file names
  • try to type cast. (e.g. OCP\IDb for $db 3497955#diff-9d6c69654961b63ec357264921a1724cR19 ) - never try to register core functions in your own DI container this will soon or later cause problems.

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
Copy link
Contributor Author

Henni commented Sep 4, 2015

@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
Copy link

LEDfan commented Sep 4, 2015

I guess this is because the changes I made here: 3497955#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 3497955#diff-ae7c5d396a800dcab2fb6f2fc7455d37R19 )

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

@LEDfan
Copy link

LEDfan commented Sep 4, 2015

@Henni is the fix working for you?

@Henni
Copy link
Contributor Author

Henni commented Sep 4, 2015

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

@BernhardPosselt
Copy link

IDb is deprecated, use IDBConnection ;)

@Henni
Copy link
Contributor Author

Henni commented Sep 4, 2015

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
Copy link
Contributor Author

Henni commented Sep 4, 2015

@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 Henni changed the title [WIP] Refactor Database Mappers Refactor Database Mappers Sep 4, 2015
@Henni
Copy link
Contributor Author

Henni commented Sep 4, 2015

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

cc @v1r0x @DJaeger @brantje @poVoq

@jancborchardt
Copy link
Contributor

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

@v1r0x
Copy link
Contributor

v1r0x commented Sep 28, 2015

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

@Henni
Copy link
Contributor Author

Henni commented Oct 18, 2015

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

@DJaeger
Copy link
Contributor

DJaeger commented Oct 18, 2015

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

@Henni
Copy link
Contributor Author

Henni commented Oct 20, 2015

@irgendwie

@irgendwie
Copy link
Contributor

LGTM 👍

irgendwie added a commit that referenced this pull request Oct 21, 2015
@irgendwie irgendwie merged commit 748c6e1 into master Oct 21, 2015
@irgendwie irgendwie deleted the refactor-db branch October 21, 2015 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants