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

Disable executing solutions on non-local environments or from non-local IP addresses on version 1.x #404

Merged
merged 4 commits into from
Aug 2, 2021

Conversation

Parsk
Copy link

@Parsk Parsk commented Jul 23, 2021

Ported code from commit #364 by AlexVanderbist to branch v1, with tests. Matter also discussed here https://github.com/facade/ignition/issues/350#issuecomment-884710367

@freekmurze
Copy link
Collaborator

Seems like the tests are failing for this one. Could you take a look at that?

@Parsk
Copy link
Author

Parsk commented Jul 25, 2021

I would seem that if ($this->app->runningInConsole()) { return $this; } here https://github.com/facade/ignition/blob/v1/src/IgnitionServiceProvider.php#L132 seems to disable Laravel builds from registering the routes? PHPUnit runs successfully on a stand alone deployment

@freekmurze
Copy link
Collaborator

I'm thinking it breaks because of changes in this PR. Tests seem to be running fine on the master branch.

@Parsk
Copy link
Author

Parsk commented Jul 25, 2021

Actually just noticed that the P7.2 - L5.8.* had successfully ran all tests, while P7.2 - L5.6.* had failed. So I really don't see a reason why it would fail. I downgraded and tested with PHPUnit 7.5.20 just to be sure, and it was all OK. Also the test code and the registered paths are identical with the current master.

Is there a possibility to run all the tests again without it stopping on first failure?

@freekmurze
Copy link
Collaborator

@Parsk
Copy link
Author

Parsk commented Jul 26, 2021

Alright, I tracked the issue down. Basically the environment setting APP_RUNNING_IN_CONSOLE was only introduced in Laravel v5.7.14, so the function runningInConsole() only looks at the php_sapi_name in older versions. So the in old versions the routes basically don't get registered.

Is it OK to skip these tests below 5.7.14? And my apologizes for wasting your time for this small change.

@freekmurze
Copy link
Collaborator

Is it OK to skip these tests below 5.7.14?
Ok, make it so! 🙂

And my apologizes for wasting your time for this small change.

No worries

@freekmurze
Copy link
Collaborator

Seems like the tests are still failing for this one 😬

@Parsk
Copy link
Author

Parsk commented Aug 2, 2021

Seems like there is some issues with the runners, those that have failed have the error "This request was automatically failed because there were no enabled runners online to process the request for more than 1 days."

Shall I push an empty commit to run the tests again?

@freekmurze freekmurze merged commit 8bac9fe into facade:v1 Aug 2, 2021
@freekmurze
Copy link
Collaborator

All good now, thanks!

@freekmurze
Copy link
Collaborator

✅ functionality already available in spatie/laravel-ignition

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.

2 participants