Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
wouterj merged 1 commit intosymfony:4.3fromragboyjr:fluent-php-di
Sep 28, 2019

Conversation

ragboyjr
Copy link
Contributor

  • 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
    excludes

Remaining: need to update the rest of the service_container files

Signed-off-by: RJ Garciarj@bighead.net

@ragboyjr
Copy link
ContributorAuthor

Before 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.

@ragboyjr
Copy link
ContributorAuthor

Any word on this? Willing to make adjustments, just want to make sure i'm not wasting anyones efforts.

@ragboyjr
Copy link
ContributorAuthor

@nicolas-grekas any thoughts?

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@ragboyjrragboyjr changed the titleWIP: [DependencyInjection] Fluent PHP DI Documentation[DependencyInjection] Fluent PHP DI DocumentationJan 18, 2019
@ragboyjrragboyjrforce-pushed thefluent-php-di branch 2 times, most recently from6547842 tocc01ad8CompareJanuary 26, 2019 19:29
@ragboyjr
Copy link
ContributorAuthor

K, 4 files left now:

service_container/  service_decoration.rst  service_subscribers_locators.rst  shared.rst  tags.rst

Might be able to finish the rest tomorrow.

@ragboyjr
Copy link
ContributorAuthor

@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.

@ragboyjr
Copy link
ContributorAuthor

@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.

@nicolas-grekas
Copy link
Member

It's up to doc mergers now (I'm not :) )

@ragboyjr
Copy link
ContributorAuthor

oh, ha, no worries! Thank you very much.

@ragboyjr
Copy link
ContributorAuthor

@javiereguiluz is there anything else I need to do here?

@javiereguiluz
Copy link
Member

javiereguiluz commentedJan 29, 2019
edited
Loading

@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!

@ragboyjr
Copy link
ContributorAuthor

@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 bePHP (Fluent), but give it a name likeConfigurator orFluent maybe. The idea being that it lets people know it's a totally different configuration file.

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.

@nicolas-grekas
Copy link
Member

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.

@javiereguiluz
Copy link
Member

I did a quick survey on Symfony's Slack. The number of answers is small ... but the results were:

  1. Almost nobody uses PHP for config and lots never heard of this fluent format.
  2. Of those using PHP, most are using (and loving) this fluent format.

@ragboyjr
Copy link
ContributorAuthor

@javiereguiluz That actually fits with my experience as well.

@ragboyjr
Copy link
ContributorAuthor

@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.

@ragboyjr
Copy link
ContributorAuthor

@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.

@javiereguiluz
Copy link
Member

@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.

@ragboyjr
Copy link
ContributorAuthor

Understood, just checking in, thanks.

@javiereguiluz
Copy link
Member

@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!

@ragboyjr
Copy link
ContributorAuthor

@javiereguiluz@xabbuh@wouterj Let me see if I can resolve the merge conflicts.

@javiereguiluz
Copy link
Member

@ragboyjr thanks a lot for all your work in this pull request. I hope it can be merged sooner than later.

@ragboyjrragboyjrforce-pushed thefluent-php-di branch 2 times, most recently from9620337 to884e5d3CompareSeptember 26, 2019 13:11
@ragboyjr
Copy link
ContributorAuthor

@javiereguiluz OK, check now please sir. I rebased off of master.

@HeahDude I updated several of the references to use the; on a new line, but i'm not sure all our switched over. I'm not sure it's worth holding up the PR at this point, we could probably have someone comb through and adjust the CS in a separate PR.

@ragboyjr
Copy link
ContributorAuthor

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?

@ragboyjrragboyjr changed the base branch from3.4 tomasterSeptember 27, 2019 02:21
@javiereguiluz
Copy link
Member

@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.

@ragboyjr
Copy link
ContributorAuthor

OK, I rebased off of master, and there aren't any conflicts. Would you prefer I rebase this off of 4.3 instead?

@javiereguiluz
Copy link
Member

Yes, please. This should go either to 3.4 or 4.3 .. so let's try if 4.3 is easier for all. Thanks!

@ragboyjrragboyjr changed the base branch frommaster to4.3September 27, 2019 09:58
- 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>
@ragboyjr
Copy link
ContributorAuthor

Rebased off of 4.3 and branch target is updated.

javiereguiluz and wouterj reacted with thumbs up emoji

wouterj added a commit that referenced this pull requestSep 28, 2019
…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
wouterj added a commit that referenced this pull requestSep 28, 2019
wouterj added a commit that referenced this pull requestSep 28, 2019
* 4.3:  [#10824] Applied some of @HeahDudes comments and added missing comments  [#10824] Fixed issues found by DOCtor  [DependencyInjection] Fluent PHP DI Documentation
wouterj added a commit that referenced this pull requestSep 28, 2019
* 4.4:  [#10824] Applied some of @HeahDudes comments and added missing comments  [#10824] Fixed issues found by DOCtor  [DependencyInjection] Fluent PHP DI Documentation
@wouterjwouterj merged commite27d27b intosymfony:4.3Sep 28, 2019
@wouterj
Copy link
Member

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!

OskarStark reacted with heart emoji

@ragboyjrragboyjr deleted the fluent-php-di branchSeptember 29, 2019 01:25
@ragboyjr
Copy link
ContributorAuthor

@wouterj@javiereguiluz woot woot!!!! So excited to see this in action now!

Glad to be able to help out with this :).

OskarStark, wouterj, and Nek- reacted with thumbs up emoji

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

@OskarStarkOskarStarkOskarStark left review comments

@HeahDudeHeahDudeHeahDude requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

Assignees
No one assigned
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.

8 participants
@ragboyjr@nicolas-grekas@javiereguiluz@wouterj@OskarStark@HeahDude@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp