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 static variable for MiddlewareUniversalName. #581

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

mlajx
Copy link
Contributor

@mlajx mlajx commented Jan 15, 2021

Hi,

I am working on a project that uses sharp as the dashboard.

I need to use it both to login to the 'central' and 'tenants', and when I found the solution (use the Feature UniversalRoutes), I noticed that Sharp does not publish its route, so I found another way to solve the problem, which would be to extend the UniversalRoutes class and then change universal to sharp_web (middleware that sharp uses on all of its dashboard routes), so that I don't have to add the 'universal' class to the routes (changing something in the vendor dir or copying all the sharp routes to laravel app).

Thinking about it, I found it interesting to do this PR by creating a public static variable, making it possible to extend the class and then just change the middleware name to something specific (in case you happen to fall into the same problem with someone).

Sorry about my english, isn't my first language.

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #581 (65aecb9) into 3.x (04193cc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                3.x     #581   +/-   ##
=========================================
  Coverage     86.94%   86.94%           
  Complexity      366      366           
=========================================
  Files           103      103           
  Lines          1088     1088           
=========================================
  Hits            946      946           
  Misses          142      142           
Impacted Files Coverage Δ Complexity Δ
src/Features/UniversalRoutes.php 92.85% <100.00%> (ø) 9.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04193cc...65aecb9. Read the comment docs.

Copy link
Member

@stancl stancl left a comment

Choose a reason for hiding this comment

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

You could solve this by just extending the class and registering the feature yourself, but this is a simple change and I agree that this makes it more consistent with the rest of the codebase that allows configuration through static properties.

The feature itself is fine, but please make these few changes.

Thanks!

src/Features/UniversalRoutes.php Outdated Show resolved Hide resolved
src/Features/UniversalRoutes.php Outdated Show resolved Hide resolved
src/Features/UniversalRoutes.php Outdated Show resolved Hide resolved
@stancl stancl merged commit 1a48725 into archtechx:3.x Jan 15, 2021
@stancl
Copy link
Member

stancl commented Jan 15, 2021

Thanks! Released as 3.4.1.

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