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

Allow https route declaration with 'https' array key and value #1655

Merged
merged 2 commits into from
Jun 19, 2019

Conversation

gazben
Copy link
Contributor

@gazben gazben commented May 7, 2019

Allow https route declaration with 'https' array key and value.

$router->group(['https' => true], function (Router $router) {
});

gazben added 2 commits May 7, 2019 12:43
Allow https route declaration with 'https' array key and value.
```
$router->group(['https' => true], function (Router $router) {
});
```
@gazben
Copy link
Contributor Author

gazben commented Jun 14, 2019

@specialtactics Can you merge this? Or do you think it will break stuff?

@specialtactics
Copy link
Member

Can't imagine any issues, but I guess we will have to see! I ran tests on a couple of projects I have and no issues, so merging.

@specialtactics specialtactics merged commit 18079c7 into dingo:master Jun 19, 2019
@Derjuju
Copy link

Derjuju commented Nov 7, 2019

Is this not a breaking change that prevent upgrading to Laravel/Lumen 6.x ?
#1689
Illuminate\Routing\Route return true/false on httpOnly if 'http' is in action array
Dingo\Api\Routing\Route return true/false on httpOnly if 'https' is in action array

Dingo\Api\Routing\Route extends Illuminate\Routing\Route

httpOnly, httpsOnly and secure methods shouldn't be overrided. They should keep the parent class logic.

Returning true with httpOnly when 'https' is set, is a non-sense... I think.

Maybe the code change was indeed for httpsOnly method and not for httpOnly method...

@gazben
Copy link
Contributor Author

gazben commented Jan 29, 2020

@Derjuju Oh my god, I messed this up. You are right. It should go to the httpsOnly/secure() function. I'll create the pull request

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.

3 participants