Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
[DependencyInjection] Fluent PHP DI Documentation#10824
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
72643e5
to10f4778
CompareBefore I start moving on to the rest of the files, would be nice if I could get some feedback to make sure this approach is fine and will work across all of our documentation. |
Any word on this? Willing to make adjustments, just want to make sure i'm not wasting anyones efforts. |
@nicolas-grekas any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, thanks for the ping and the PR.
6547842
tocc01ad8
CompareK, 4 files left now:
Might be able to finish the rest tomorrow. |
@nicolas-grekas OK, all of the files have been updated, and I was able to build the site locally, and it all checked out. This is ready to go live IMHO. |
@nicolas-grekas is there anything else i need to do to get this merged? Would love to start seeing the fluent php configuration show up on the site so I can link to it for the devs i work with. |
It's up to doc mergers now (I'm not :) ) |
oh, ha, no worries! Thank you very much. |
@javiereguiluz is there anything else I need to do here? |
javiereguiluz commentedJan 29, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ragboyjr you did everything great and I thank you for that! However, merging this is a bit complicated. Why? Because we already have PHP config in all examples and here we are adding ... another PHP config. Showing two similar, but slightly different, configs is always confusing to users. Why two? Which one is the right one? What if I choose one and the right one is the other one? etc. Instead of showing both, I think we should replace the existing PHP config by the new PHP config. However, is this fluent config polished enough? Is it used enough? I don't hear people asking questions about this or saying that they use it. I'm going to ask about this to the Docs Team and we'll be back with a reply. Thanks! |
@javiereguiluz I appreciate the feedback! My only two cents is that the PHP Fluent config won't ever get "used" or "polished" enough unless it's documented properly. A lot of that information in the docs I just got from using it, IDE auto-complete, and checking source code, and I've found that it's hard to sell a team on trying to use this format when there isn't any documentation on it except for a blog post by the symfony team for the 3.4 release. All that to say, I think before it could ever replace the other PHP format, it would need to at least be documented so that people could use it, and we can work out the kinks. I totally understand your concern regarding showing both php types would be confusing; however, I'm not sure dropping the default php version would be good since it does provide valuable information on how to interact with the DI container. It's especially helpful when using Compiler Passes. The only solutions I could come up with would be to change the name of it to not be The other would be that we put this PR on the back burner, and we create a dedicated page show casing each of the individual parts of DI configuration using the PHP fluent format, and it could be apart of the learn more. Then after some time, if we feel like PHP fluent is taking off etc etc, then we can add it into the main docs or something. |
I'd be 👍 to fade out the old PHP format - it doesn't provide anything over the fluent one, except maybe low level access to primitive data structures. |
I did a quick survey on Symfony's Slack. The number of answers is small ... but the results were:
|
@javiereguiluz That actually fits with my experience as well. |
@javiereguiluz Let me know what you want to do, willing to update this however you want. The hard part was just creating the mapped config. If I have to shuffle stuff around or remove some stuff, it'll be quick. |
@javiereguiluz ping, here to help and contribute any you need/like. Would love to see the comprehensive fluent php docs show up on the SF docs site in any shape or form. |
@ragboyjr sorry for the lack of response. We haven't forgotten about this, but we're still debating about this internally in the Symfony Docs team. Thanks. |
Understood, just checking in, thanks. |
@xabbuh@OskarStark@wouterj after reviewing this ... I'm afraid I cannot merge it. I never use this PHP format, so I can't fix any of the multiple conflicts that will happen when upmerging this to 4.3, 4.4 and master. Would any of you available for this? Keep in mind that the merging will be very complex and time-consuming. Thanks! |
@javiereguiluz@xabbuh@wouterj Let me see if I can resolve the merge conflicts. |
@ragboyjr thanks a lot for all your work in this pull request. I hope it can be merged sooner than later. |
9620337
to884e5d3
Compare@javiereguiluz OK, check now please sir. I rebased off of master. @HeahDude I updated several of the references to use the |
gah,@javiereguiluz I rebased off of master and not 3.4 :(. When rebasing off of 3.4 i get another set of conflicts :'(. Are we sure we want to target this documentation for 3.4? Or maybe a later branch? |
@ragboyjr you've spent so much time here that I feel bad if I keep asking things .... but yes, adding this only to 4.3 and higher branches would help a lot when merging it. |
OK, I rebased off of master, and there aren't any conflicts. Would you prefer I rebase this off of 4.3 instead? |
Yes, please. This should go either to 3.4 or 4.3 .. so let's try if 4.3 is easier for all. Thanks! |
- Updated DI configuration references to include php-fluent-di- Updated _build/conf.py to include codeblock for php-fluent-di- Updated existing di resource loading to refer to latest sf 4.2 excludesSigned-off-by: RJ Garcia <rj@bighead.net>
Rebased off of 4.3 and branch target is updated. |
…boyjr)This PR was merged into the 4.3 branch.Discussion----------[DependencyInjection] Fluent PHP DI Documentation- Updated service_container.rst to include php-fluent-di- Updated _build/conf.py to include codeblock for php-fluent-di- Updated existing di resource loading to refer to latest sf 4.2 excludesRemaining: need to update the rest of the service_container filesSigned-off-by: RJ Garcia <rj@bighead.net>Commits-------e27d27b [DependencyInjection] Fluent PHP DI Documentation
Hi@ragboyjr! What an incredible job did you do with this PR. I cannot imagine the effort you put into this. That's makes it even worse that we ignored it for so long. Fortunately, it's now merged into 4.3/4.4/5.0. While merging, I fixed some issues found by our review tool in3ccce23 and I checked all comments by@HeahDude and applied some of them - as well as some things I discovered - in2a5b114. Thanks a lot for your quick responses and actions this week! |
@wouterj@javiereguiluz woot woot!!!! So excited to see this in action now! Glad to be able to help out with this :). |
excludes
Remaining: need to update the rest of the service_container files
Signed-off-by: RJ Garciarj@bighead.net