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 JSON:API support for Drupal #705

Merged
merged 1 commit into from
May 10, 2019
Merged

Add JSON:API support for Drupal #705

merged 1 commit into from
May 10, 2019

Conversation

samgranger
Copy link
Contributor

This change fixes issues with using the JSON:API in Drupal.

@mattstauffer
Copy link
Collaborator

@samgranger Since I don't use Drupal it's hard to check this. Could you ask some other Drupal/Valet folks to check this out and give feedback here for me?

@mattstauffer
Copy link
Collaborator

@samgranger Ping--anyone you know who can test it who's a Drupal dev using Valet?

Copy link

@shadcn shadcn left a comment

Choose a reason for hiding this comment

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

@samgranger @mattstauffer Tested. This worked for me. Fixes #771. LGTM.

@mattstauffer mattstauffer merged commit f22cb36 into laravel:master May 10, 2019
@@ -57,7 +57,7 @@ public function frontControllerPath($sitePath, $siteName, $uri)
{
$sitePath = $this->addSubdirectory($sitePath);

if (!isset($_GET['q']) && !empty($uri) && $uri !== '/') {
if (!isset($_GET['q']) && !empty($uri) && $uri !== '/' && strpos($uri, '/jsonapi/') === false) {
Copy link

Choose a reason for hiding this comment

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

👋 I'm the JSON:API maintainer in Drupal.

Bear in mind that the /jsonapi path is configurable. So is anyone changes the defaults this may break.

Copy link

Choose a reason for hiding this comment

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

@e0ipso oh yeah. I completely missed that. Funny thing is, IIRC, I wrote the patch to make this configurable. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for sharing, y'all! Sooo.. is there any way to fix this driver or is that just not going to be possible since it's totally variable?

Choose a reason for hiding this comment

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

I have a custom prefix and good to have this issue to know where should I look for in valet. thanks!

@chrfritsch
Copy link

This fix only works for the contrib module. There is still #771

@mattstauffer
Copy link
Collaborator

@chrfritsch Thanks for bringing that back up! I'd love for anyone who knows how this all works to PR in a fuller fix.

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.

6 participants