Skip to content

Rewrite domain restrictions#14

Open
krve wants to merge 5 commits into
viacreative:masterfrom
krve:master
Open

Rewrite domain restrictions#14
krve wants to merge 5 commits into
viacreative:masterfrom
krve:master

Conversation

@krve

@krve krve commented Mar 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

mrterryh commented Mar 29, 2017

Copy link
Copy Markdown
Collaborator

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

@jedkirby

jedkirby commented Mar 30, 2017

Copy link
Copy Markdown

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

@krve

krve commented Mar 30, 2017

Copy link
Copy Markdown
Contributor Author

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

@jedkirby jedkirby left a comment

Copy link
Copy Markdown

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

Jaspur commented Mar 31, 2017

Copy link
Copy Markdown

But not really backwards compatible for people using current version?

@mrterryh

mrterryh commented Mar 31, 2017

Copy link
Copy Markdown
Collaborator

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

@krve

krve commented Mar 31, 2017

Copy link
Copy Markdown
Contributor Author

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

@mrterryh

mrterryh commented Mar 31, 2017

Copy link
Copy Markdown
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

krve commented Apr 1, 2017

Copy link
Copy Markdown
Contributor Author

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

@mrterryh

mrterryh commented Apr 5, 2017

Copy link
Copy Markdown
Collaborator

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

krve commented Apr 5, 2017

Copy link
Copy Markdown
Contributor Author

@mrterryh yeah, that seems like a good idea.

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

mrterryh commented May 3, 2017

Copy link
Copy Markdown
Collaborator

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

DODMax commented Jun 13, 2017

Copy link
Copy Markdown

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

krve commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

@DODMax on what version of Laravel?

@DODMax

DODMax commented Jun 14, 2017 •
edited
Loading

Copy link
Copy Markdown

@krve on 5.4.25
I'm using commit 05fe056

@krve

krve commented Jun 14, 2017

Copy link
Copy Markdown
Contributor Author

@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

krve commented Jun 14, 2017

Copy link
Copy Markdown
Contributor Author

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

@DODMax

DODMax commented Jun 15, 2017

Copy link
Copy Markdown

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

@RStrydom RStrydom mentioned this pull request Sep 5, 2017
Open
@krve

krve commented Nov 24, 2017

Copy link
Copy Markdown
Contributor Author

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

@LoganGray

LoganGray commented Jul 28, 2018

Copy link
Copy Markdown

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

karmendra commented Aug 22, 2018

Copy link
Copy Markdown

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

1 more reviewer
@jedkirby jedkirby jedkirby approved these changes
Reviewers whose approvals may not affect merge requirements

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