-
Notifications
You must be signed in to change notification settings - Fork 109
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
Use JsonApi in routes to register adapters #50
Use JsonApi in routes to register adapters #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nitpicking, hope you don't mind. :)
Not sure if @lindyhopchris automates this, but if not, we might as well help out. 👍
src/Adapters/EloquentAdapter.php
Outdated
* @param string $model | ||
* @param string $keyName | ||
*/ | ||
public function addResource($resourceType, $model, $keyName = NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use lowercase null
in default param value
public function addResource($resourceType, $model, $keyName = null)
src/Adapters/EloquentAdapter.php
Outdated
*/ | ||
public function addResource($resourceType, $model, $keyName = NULL) | ||
{ | ||
$this->map[$resourceType] = $model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add line between this statement and if
-statement.
$this->map[$resourceType] = $model;
if (! is_null($keyName)) {
$this->keyNames[$resourceType] = $keyName;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note the extra space between if
and the start paran (
.
src/Services/JsonApiService.php
Outdated
$adapterObject = $this->container->make($adapter); | ||
|
||
// Check if adapter recognises resource type | ||
if(! $adapterObject->recognises($resourceType)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add space between if
and the start paran (
:
if (! $adapterObject->recognises($resourceType)) {
src/Services/JsonApiService.php
Outdated
* @param array $options | ||
* @return ResourceRegistrar | ||
*/ | ||
public function eloquentAdapter($resourceType, $model, $keyName = NULL, $controller = null, array $options = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the default null
parameter lowercase:
public function eloquentAdapter($resourceType, $model, $keyName = null, $controller = null, array $options = [])
Done :) |
Nice @pedroluislopez! 💯 |
Sorry about this, but I don't think this will work if you cache your routes. Have you tried it with cached routes? Also, I'm not sure it solves any problem. You say that it avoids putting stuff in configuration, but the config is there for a reason. |
Effectively it doesn't work with About the purpose of this PR: The modern frameworks do implement automatic features in order to avoid complex configuration files that are modified, usually, by several developers. For example, we work in the same project five developers. Laravel has this philosophy. In fact, you use this philosophy in two aspects:
However, effectively this solution doesn't work. If you like, I can offer other solutions based on the schema that you define for I will close this PR, we will do other PR with other solutions and discuss about them if you like. |
Wherever we can move to automation, within the ways that Laravel 5 provides, I am supportive of it. The thing to remember is that this package is still in development (hence no Please remember that in any automation you are proposing it must be capable of supporting multiple API definitions mapping to one set of models. I.e. the package has to support having multiple APIs that expose models in different ways. I'd also strongly suggest you submit a proposal for the change via an issue before writing any code. That way we can discuss the idea before any code is written. We are already working on changes to this package, which I've written up here so you can take a look: |
This feature allow register adapters in the routes PHP file when you define JSON API resources. This feature avoid use a configuration file for register adapters and/or Eloquent adapter mappings. Example: