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

Add compatibility for Laravel 5.5 #26

Merged
merged 4 commits into from
May 12, 2020
Merged

Add compatibility for Laravel 5.5 #26

merged 4 commits into from
May 12, 2020

Conversation

alquerci
Copy link
Contributor

@alquerci alquerci commented May 12, 2020

Hello,

I found your library from nuwave/lighthouse#311, and I see that it does not depends on HTTP request that's excellent for an API usage such as GraphQL.

I have a project using Laravel v5.5 without time frame for a framework upgrade.

Why

Sadly I see that currently Laravel version 5.5 is not supported on this library.

So I checkout quickly the effort to make it compatible.

I do not see a direct dependency on the implementation for ...\Resource or ...\JsonResource.

How

The class Illuminate\Http\Resources\Json\Resource extends ...\JsonResource since Laravel version 5.5. But it is removed on version 7.0.

What I see about the limitation for Laravel 5.5 is only on tests suite.

Question

Does this compatibility can going to be possible?

Next

I also try to add Laravel 7 on travis.ci tests but I got this result with issue on date format.
https://travis-ci.org/github/alquerci/lampager-laravel/jobs/686051996

@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 653421c on alquerci:backport-to-laravel-5-5 into afd69ad on lampager:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d1a8abc on alquerci:backport-to-laravel-5-5 into afd69ad on lampager:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d1a8abc on alquerci:backport-to-laravel-5-5 into afd69ad on lampager:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d1a8abc on alquerci:backport-to-laravel-5-5 into afd69ad on lampager:master.

Copy link
Member

@mpyw mpyw left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the PR!

Unfortunately, Resource is a soft-reserved word in the current PHP version. If we use the word in the source codes, it may cause Parse Error in the future versions. And Laravel 5.5 (LTS) will end its support term on this August.

Currently I'm not willing to merge the suggestion, sorry. Thank you anyway!

@alquerci
Copy link
Contributor Author

alquerci commented May 12, 2020

Ha, yes, I miss this point about resource.

Pushed a better solution.


But you are free to close this PR if the following point take precedence.

Laravel 5.5 (LTS) will end its support term on this August.

Copy link
Member

@mpyw mpyw left a comment

Choose a reason for hiding this comment

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

LGTM

@mpyw mpyw merged commit d76dea4 into lampager:master May 12, 2020
@alquerci alquerci deleted the backport-to-laravel-5-5 branch May 12, 2020 11:50
@alquerci
Copy link
Contributor Author

alquerci commented May 12, 2020

@mpyw Thank you!

Nice picture.

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.

None yet

3 participants