-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add missing autoload include for console components page #4409
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
Conversation
And better explain where files should be placed in order to be auto-loaded.
"psr-0": {"": "src/" } | ||
} | ||
|
||
} |
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.
I'm 👎 on this. It is explained in detail in the referenced installation instructions how to install new components.
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.
The part missing from the docs is actually the autoload
-- or is mentioned in another place?
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.
The autoload of the required packages already handled by Composer. You don't need to do anything else. This autoload config sets up an autoloader for the classes of the app/library itself. That's not in the scope of using the component and it is already explained perfectly in the Composer documentation.
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.
- So if we remove the
"autoload"
where should theGreetCommand.php
be created? - Also we'd still need to
require_once './vendor/autoload.php';
from theapplication.php
, right?
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.
Where you want it to be. The point is, you need to define the autoload, but that's not the scope of this article. In documentation, each article and section has its own scope. The Components documentation explains you how to use the components in a standalone PHP project. Telling you how to set-up such a project and how to organize your files and autoloading is not in the scope of these articles. That is a perfect topic for a blog post or the Composer documentation.
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.
The docs for the SE as great, since that's the goal of the Symfony docs. The complete SE is in the scope of the Symfony documentation.
Structuring a project and setting up one isn't. So yeah, you have to search for it. Not sure if it's something we should improve.
Fortunate for you, I'm not the only guy who decideds these things. I'll wait for their comments too. /cc @weaverryan
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.
Hmm, all great points!
When I was updating some component docs the other day, it did occur to me that we're leaving out the detail of requiring the autoloader (so making the docs not really copy-pastable). So that does seem like a small detail we could add to help people. For example, if you look at the Finder component, we would just need to add the require
for the autoload.php
in the first code block and maybe just a few others http://symfony.com/doc/current/components/finder.html. I'm thinking whenever we show a full example (i.e. include the use
statement), we could also include the require
line and a little // foo.php
filename comment at the top. What do you guys think about this part?
Second, there is this autoloading proposal. The good news is, I don't think this affects most component docs (we don't ever create any objects in the Finder docs, for example). If I do a git grep "namespace "
, it only shows 7 of the components. For these, I think it might be nice to add a quick note near-ish the first related code example that mentions this autoloader code. So, what do you guys think about this part?
Thanks!
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.
@weaverryan both suggestions sound good to me, as they will assure the a dev will be able to get a working code with the component without leaving the page.
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.
@weaverryan let's decide the target audience for the components docs. That always makes a discussion easier than discussing with people all different target audiences in mind.
My target audience for the components docs is 3 people:
- First there are people who are linked to some related usage article (for instance, the Translation usage). These people use the fullstack framework and just want to know usage
- Secondly, the components page attracks people who want to use the component standalone.
- At last, people who want to know more about Symfony internals visit the components articles.
(1) and (3) use the full-stack framework, these autoloading things aren't targeted for one.
Then we are left with (2). I always imagine these people to either already have a project or are at the start of bootstrapping one. They just go to the component docs because they know/found that this component will solve the problem they have.
I think there are almost none users who go the components docs, find a nice component and create a project from it.
The means that these docs are solving the problem of "how to use the component to solve the problem"? That excludes anything related to "how to bootstrap a project?", "what is the best library structure?" and "how to use Composer?".
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.
@wouterj Very nice breakdown - I couldn't have thought of it better! And I agree that we have all 3 audiences and also that we don't need to teach people how to bootstrap a project, structure things or use Composer.
So, where does that leave us? I think:
A) We show a require of the autoload.php
at the very top of every component docs in the installation section. I'm basing this off of Guzzle, which is a library that a lot of people use: http://guzzle.readthedocs.org/en/latest/overview.html#installation
B) We include a patch like this one for the rare case where we're assuming you have some auto-loading setup. We do this in the installation section.
For the 3 groups:
- Full-stack FW users will not (hopefully) bother with the installation. So they hit no new problems
- People wanting to include this in their non-FW project will have the autoloader and autoloading details if they need them.
- People wanting to learn more about the internals will skip past installation as well.
Cheers!
Sorry for waiting with my response for so long. I'm in favor of (A), show it in the installation section just like Guzzle does it. @amitaibu can you maybe update the PR to add this for each component installation section? Otherwise, feel free to close this PR and open an issue for it instead so someone else can do it. |
I'm trying to finish this PR in #5385. Thanks. |
closing in favor of #5385 |
…oad file (javiereguiluz) This PR was merged into the 2.3 branch. Discussion ---------- Added a note about the need to require Composer's autoload file | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | - This PR tries to finish #4409. I don't know if I applied correctly the ideas proposed by @weaverryan and @wouterj in that PR. Commits ------- 7f1ec8a Added a note about the need to require Composer's autoload file
And better explain where files should be placed in order to be auto-loaded.