[5.4] Make consistent the disuse of helper functions within ServiceProviders

[collegeman]

Following on #18506, this branch further refactors DatabaseServiceProvider, NotificationServiceProvider, and PaginationServiceProvider, and updates the signatures of Application::databasePath(), Application::configPath(), Application::bootstrapPath(), and Application::basePath() to support a $path argument, making consistent the path accessing part of Application's API.

Thank you for your consideration.

[taylorotwell]

Is there going to be more of these changes? TBH I'm not 100% fired up about changing all this to support what I would consider to be a pretty gnarly edge case.

[collegeman]

I admit it is gnarly, and I really appreciate you taking a moment to consider the changes. The alternative to these changes is to have slightly modified ServiceProvider implementations in my project, which feels even gnarlier (though it does patch the problem, absent modifying the framework).

I'm pretty sure I nailed everything with this one—I failed to take the time to review all of the core ServiceProviders before making my first pull request.

Thank you again for your time.

[laurencei]

I would suggest this at least goes into master/5.5 and not 5.4 (if it is going to be accepted)?

Even if not technically a breaking change; gives a reasonable chance of any issues being caught during a planned upgrade in case people are doing another gnarly stuff that could impact the other way in their own applications.

[collegeman]

Hoping against all hope for both 5.4 and acceptance since the consistent changes I requested to Application::resourcePath() were merged into 5.4 this morning. But of course, I understand if this doesn't suit. The most important changes outlined here have to do with dis-using the helpers inside ServiceProviders.

[taylorotwell]

But it's not just not using helper functions, it's changing the entire rules about what can even be done in a service provider from the framework's perspective. We would now have to always assume that people are running in this strange multi-application setup for all eternity. I'm not sure I want to take on that mental overhead.

[collegeman]

My specific use case is esoteric; that point is not debatable, and your long view is valid. As long as my patch approach works, I can absorb the long-term mental work you would prefer to avoid—I can accept that responsibility. The helper functions are, by design, able to be overridden. If I have a higher order global function named "database_path", it will short circuit the dependence DatabaseServiceProvider places upon it. I don't think this was your intention, since the helpers documentation does not specify this overridability as a feature. And since the helpers merely alias functions that are defined in Application, overriding the helpers to change their behavior wouldn't be the correct place to make such a customization; I would imagine you would instead direct people to subclass Application, or write their own implementation of Container. So in a sense the overridability of the helpers and the ServiceProvider's dependence upon them creates a vector through which bugs can be introduced. This is the only other argument I can make, other than my own esoteric use case, for ending the practice of calling helpers from within ServiceProviders. I don't, however, want to misrepresent my appreciation for what you've done here—it really knows no bounds. The helper functions really do save time and keep code concise that would otherwise be made less so by the necessity of coding all application behavior with object oriented syntax. Shortcuts are awesome; I think they should be reserved for applications build on top of the framework. What I am trying to do is test on the way to standardizing a pattern for using Laravel to build plugins for WordPress. I've written my own Container loosely based on Lumen, which is why arbitrarily replacing core ServiceProviders is a viable strategy for me. But doing so breaks the clean dependencies on your work which made this strategy so attractive to me. Ultimately I'm trying to transform WordPress into a platform for shipping some cool products. I want to build those products using the solutions you've developed. I may be on a fool's errand, but the degree to which I've had to study your work and WordPress has been paying huge dividends into my work. Thank you again for your attention to this. I know you have to make careful choices about how you spend it.

On Mon, Mar 27, 2017 at 5:34 PM Taylor Otwell ***@***.***> wrote: But it's not just not using helper functions, it's changing the entire rules about what can even be done in a service provider from the framework's perspective. We would now have to always assume that people are running in this strange multi-application setup for all eternity. I'm not sure I want to take on that mental overhead. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18521 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHV_Fy-VOTtd5MyLNGFuNYB03b7LuzSks5rqCtjgaJpZM4MqZHy> .

-- Aaron Collegeman Owner, Fat Panda 540-335-9013 [email protected]

[taylorotwell]

So, just so I understand, you don't know if your use case will work when this is merged? This is just to allow you to test some idea you have?

[collegeman]

No, I know my use case works. I don't know if the things I want to build will be popular. I meant market testing.

On Tue, Mar 28, 2017 at 8:53 AM Taylor Otwell ***@***.***> wrote: So, just so I understand, you don't know if your use case will work when this is merged? This is just to allow you to test some idea you have? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18521 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHV_JLdOdj98LQKzdnykFy1tGiqIW6Nks5rqQLNgaJpZM4MqZHy> .

-- Aaron Collegeman Owner, Fat Panda 540-335-9013 [email protected]

[taylorotwell]

I'll merge this one but I don't plan on devoting a lot of effort to making sure this use case works. 😬

[collegeman]

I wouldn't expect you to. Still, your time and attention are greatly appreciated. ❤

[taylorotwell]

This broke Lumen.

[collegeman]

Ouch! I'm on drive time now. Can I research and address, or are we at an impasse?

[collegeman]

My apologies for not being able to respond before now. I would be more than happy to debug whatever issue cropped up as a result of these changes. Do you have any more details? My sincerest apologies for the trouble—how can I make it up to you?