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.

Sign up for GitHub

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

Jump to bottom

Rewrite domain restrictions #14

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Rewrite domain restrictions #14

wants to merge 5 commits into from

Conversation

krve
Copy link
Contributor

@krve krve commented Mar 28, 2017 •
edited
Loading

I've made a draft of how the domain validation could work according to #1

krve added 3 commits March 28, 2017 13:17
@krve
Change domain restriction-system
d9a1bb8 
This will allow for a more configurable system where
you can specify a full domain name, all subdomains,
a tld and port numbers.
@krve
Update readme
db54660
@krve
Merge branch 'master' of github.com:krve/sudo-su
208cae0
@krve krve mentioned this pull request Mar 28, 2017
Open
@mrterryh
Copy link
Collaborator

This looks awesome, I'm going to have a proper look in the morning. Thanks so much for this @krve!

@jedkirby
Copy link

Would be nice if we could add some tests into the domain restrictions to ensure it's working correctly

@krve
Copy link
Contributor Author

krve commented Mar 30, 2017

I'll see if I can't create some tests for the domain restrictions this weekend.

@krve
Add tests for DomainRestricter
05fe056 
I also extracted the domain restricter to a specific class
to keep it self-contained and make it easier for testing purposes
jedkirby
jedkirby approved these changes Mar 30, 2017
View reviewed changes
Copy link

@jedkirby jedkirby left a comment

Choose a reason for hiding this comment

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

Nice work @krve, looks much better 👍

@Jaspur
Copy link

Jaspur commented Mar 31, 2017

But not really backwards compatible for people using current version?

@mrterryh
Copy link
Collaborator

Sorry, been hectic at work today! Will definitely review this early next week.

@krve
Copy link
Contributor Author

krve commented Mar 31, 2017

Completely forgot about that @Jaspur. Will look into it this weekend to make sure it has some backwards capability.

@mrterryh
Copy link
Collaborator

If you think backwards compatibility is going to be an issue, I would be happy to consider making this a breaking change and publishing a new version of the package.

@krve
Copy link
Contributor Author

krve commented Apr 1, 2017

Yeah, I think that's a better idea. This way we don't have code specifically for an old version of the package.

@mrterryh
Copy link
Collaborator

mrterryh commented Apr 5, 2017

Cool. I'm thinking about merging all these recent PRs and just releasing a new 2.0 version. Do you think that's sensible @krve?

@krve
Copy link
Contributor Author

krve commented Apr 5, 2017

@mrterryh yeah, that seems like a good idea.

@krve krve mentioned this pull request Apr 6, 2017
Open
@mrterryh
Copy link
Collaborator

mrterryh commented May 3, 2017

Just to let you know, I've still got this on my to-do list! Super hectic at work. I'll look ASAP, I promise!

@DODMax
Copy link

DODMax commented Jun 13, 2017

I tried your package as I need custom domains but I get the following error:

Type error: Argument 1 passed to VIACreative\SudoSu\ServiceProvider::__construct() must be an instance of VIACreative\SudoSu\DomainRestricter, instance of Illuminate\Foundation\Application given, called in E:\xampp\htdocs\mfund\vendor\laravel\framework\src\Illuminate\Foundation\Application.php on line 612

@krve
Copy link
Contributor Author

krve commented Jun 13, 2017

@DODMax on what version of Laravel?

@DODMax
Copy link

DODMax commented Jun 14, 2017 •
edited
Loading

@krve on 5.4.25
I'm using commit 05fe056

@krve
Copy link
Contributor Author

krve commented Jun 14, 2017

@DODMax I'll take a look at it today when I'm home and fix the issue.

@krve
Change DomainRestricter initialization
e935c50 
Moved the initialization to the boot method and make use of laravels Dependency Injection.
@krve
Copy link
Contributor Author

krve commented Jun 14, 2017

@DODMax Just pushed an update that should fix the issue 👍

@DODMax
Copy link

DODMax commented Jun 15, 2017

@krve Works like a charm. Thanks for the PR!

@RStrydom RStrydom mentioned this pull request Sep 5, 2017
Open
@krve
Copy link
Contributor Author

krve commented Nov 24, 2017

@mrterryh - You wanna merge this? I kinda want to get this one merged so I can work on some of the other issues.

@LoganGray
Copy link

I'm very surprised nothing ever happened with this. I just hacked it a bit to work with an existing application that is still basically Laravel 5.2. And while it had some issues.. I was able to get it to do what I wanted.
I really like the concept. Does anyone know if this has been improved and one of these many forks is actually preferred?

@karmendra
Copy link

This error has been a pain, and i see this PR addresses it well, any ETA on when this is going to be released. I prefer not to use a fork as it will be a pending task to switch back to original repository, else would miss all the other enhancements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedkirby jedkirby jedkirby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@krve @mrterryh @jedkirby @Jaspur @DODMax @LoganGray @karmendra