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

Phalcon storage #743

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

lucasantarella
Copy link

@lucasantarella lucasantarella commented Jun 29, 2016

Hi, I’ve created an integration between your library and a popular REST framework called Phalcon. I personally use the code that I’ve created in my own API, so I can attest to the fact that it works. I’m doing this to share with everyone else who uses Phalcon. Please consider adding this to your library. I know this would have saved me a huge headache when implementing this into my API.

@bshaffer
Copy link
Owner

Hey this is really cool! Nice work. Looks like it's essentially an ORM for the models this library uses. A few things:

  • tests
  • code standards (should be consistent with the rest of this library)

Does this only work for APIs or can it be used for local PDO as well?
There is a lot of boilerplate/crud code that doesn't really belong in this repo. Is there any way to get around that, or is it required for the library to function?

@lucasantarella
Copy link
Author

lucasantarella commented Jun 29, 2016

Hey,
Thanks for taking a look. I'll get working on the tests, that shouldn't be too big of a problem.

I tried to adhere as closely as I could to the PDO Storage code. Could point out some places were I should make the code more consistent with the rest of the library?

By boilerplate code, do you mean the model files? Those are needed for the library to function. I don't think there is anyway to get around having those model files. I can tone down the PHPDoc if that helps at all.

This library works with PDO inherently. Really, it works with a ton of storage options (MySQL, PostgreSQL, SQLite, and Oracle).

@lucasantarella
Copy link
Author

lucasantarella commented Jun 29, 2016

For reference, here is the Phalcon documentation about the models and how to use them. The other option could be to use PHQL (Phalcon's dialect of SQL) to use in the library instead of all the Model::find([]) lines.

* @param mixed $parameters
* @return OauthUsers
*/
public function getUser($parameters = null){
Copy link
Owner

Choose a reason for hiding this comment

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

brace should be on a new line

@bshaffer
Copy link
Owner

bshaffer commented Jul 1, 2016

Thanks again for your work on this!

It would definitely be preferable to only have the single Phalcon storage class, without generating all the dependent wrapper classes for each table. This would keep the API consistent (i.e. passing arrays of data rather than objects) between the storage types.

Also, add the Phalcon storage class to BaseTest and it'll receive all the same testing as the rest of the storage classes.

@lucasantarella
Copy link
Author

Even with PHQL, there still needs to be a model class defined for each table, even though it would be significantly cut down (only the public vars would need to be defined and the initialize() function). I could put all these classes at the bottom of the Phalcon.php file and have all the classes in one file, maybe that would help with the consistency of the repo. Let me know what you think.

@lucasantarella
Copy link
Author

I believe the test is going to fail straight-up right now. I have to add the TravisCI config for Phalcon since it's a PHP extension. Also, it's not compatible with PHP7.

@bshaffer
Copy link
Owner

bshaffer commented Jul 1, 2016

No need to add them all to the same file! If we need them then we'll keep them. You can skip the tests in PHP7 and add the extension in .travis.yml.

* Added Travis CI installer for Phalcon

* Travis CI config for phalcon

* Refactored to the corresponding directory for the Phalcon classes

* Added phalcon autoload tests

* Test test

* Get client details test

* Init DB for phalcon test

* Refactored di connection

* More tests

* Disabled throwing an exception

* Setup methods

* Fixed table names

* Finalize tests
@lucasantarella
Copy link
Author

@bshaffer Hope you enjoyed the holiday! I'm working with one of the Phalcon dev's to get PHP 5.3 build working in Travis CI. Seems to be a travis problem about not installing the Phalcon extension. Otherwise, the tests should work fine. The library will work on any Phalcon 2.x.x and up installation, and that includes PHP >= 5.3

lucasantarella and others added 2 commits July 5, 2016 12:23
* Added Travis CI installer for Phalcon

* Travis CI config for phalcon

* Refactored to the corresponding directory for the Phalcon classes

* Added phalcon autoload tests

* Test test

* Get client details test

* Init DB for phalcon test

* Refactored di connection

* More tests

* Disabled throwing an exception

* Setup methods

* Fixed table names

* Finalize tests

* Min php version set to 5.3.21

* Removed manually setting the phalcon version

* Php 5.3.29 for Phalcon Zephir

* Added dev packages

* using sudo

* Installing 2.0.x of phalcon

* Removed PHP7 and HHVM from config

* Using dynamic table names for the library

* Using getShared instead of getRaw

* Fixed recursion depth

* Using get instead of getShared

* Using function

* Using special config class

* Added special phalcon conf class

* Using containerbased infrastructure

* Fixed not sending the class in the di

* Moved the conf class back to the Phalcon main class
- phpenv config-rm xdebug.ini || return 0
- composer install --prefer-source --no-interaction
- vendor/bin/install-phalcon.sh 2.0.x
- phpenv config-rm xdebug.ini || return 0
install:
- composer install --no-interaction
Copy link
Owner

Choose a reason for hiding this comment

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

why is composer install also in install? How about you move the phalcon installation to install instead?

* Using array instead of class

* Removed unused PhalconConf class
@lucasantarella
Copy link
Author

Stumbling through the documentation, I found that you can use a function instead of a class. This allows the return array() to be used and negates the need for a special class. Sorry about the confusion! I didn't know myself.

@lucasantarella
Copy link
Author

Fixed all tests.

@lucasantarella
Copy link
Author

Hi @bshaffer I finally finished the tests and logic for this PR. What do you think about merging?

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.

2 participants