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 support for multiple model directories #246

Closed
wants to merge 4 commits into from

Conversation

koenpunt
Copy link
Collaborator

As the title says, support for multiple model directories. Could be a solution for #163

The set_model_directory and get_model_directory functions still work as they are proxied to set_model_directories and get_model_directories.

Note, view the diff with ?w=1, because the <CR>'s (carriage returns) are removed.

$model_directories = $this->get_model_directories();
return array_shift($model_directories);
}

Copy link
Owner

Choose a reason for hiding this comment

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

OCD: remind removing the superfluous line?

@jpfuentes2
Copy link
Owner

Would it be too much to ask to also add a test for this change under this?

@koenpunt
Copy link
Collaborator Author

Rebased and updated with your comments, will look into adding an additional test.

@koenpunt
Copy link
Collaborator Author

On second thought, the test uses set_model_directory, which is nothing less than a proxy to set_model_directories ($this->set_model_directories(array($dir));). But if you prefer a test I can add one later.

@jpfuentes2
Copy link
Owner

Please, if you don't mind. I would very much appreciate a test. This is a widely used library and a test can also serve the purpose of me not breaking your changes later on down the road :).

@koenpunt
Copy link
Collaborator Author

@jpfuentes2 ping, added tests.

@ryangurn
Copy link

ryangurn commented Jul 9, 2013

how do i get these changes into my application?

@koenpunt
Copy link
Collaborator Author

koenpunt commented Jul 9, 2013

If you're using a git checkout of php-ar you can simply merge my branch (git pull https://github.com/fetch/php-activerecord multiple_model_dirs). Otherwise you can apply the diff to your current source: https://github.com/kla/php-activerecord/pull/246.patch (patch -p1 < 246.patch)

@ryangurn
Copy link

ok cool thanks

@al-the-x
Copy link
Collaborator

Looks like this PR supersedes #330... And there's more discussion here.

@al-the-x
Copy link
Collaborator

Should definitely consider this for 1.1, since it's a long outstanding issue.

@al-the-x al-the-x mentioned this pull request Aug 1, 2013
@koenpunt koenpunt closed this Jul 17, 2014
@koenpunt koenpunt deleted the multiple_model_dirs branch July 17, 2014 09:06
This was referenced Jul 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants