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

Clarify tip for creating a new AppBundle#4702

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
weaverryan merged 1 commit intosymfony:2.3fromxelaris:patch-9
Jan 3, 2015

Conversation

xelaris
Copy link
Contributor

QA
Doc fix?yes
New docs?no
Applies toall
Fixed tickets

The tip seems to be confusing, since the AppBundle is also generated withsymfony new blog 2.3.23 (and 2.5.8). In addition the mentionedgenerate:bundle command does not work in SF 2.3 SE, since it stops with aThe namespace must contain a vendor namespace error.

@javiereguiluz
Copy link
Member

@xelaris thanks for reporting this issue.

You are right about the error generated bygenerate:bundle in Symfony 2.3.x. However, there is a finished (but not merged) fix to this problem:sensiolabs/SensioGeneratorBundle#312 Once this pull request gets merged, Symfony 2.3.x will be able to generate bundles without vendors. That's why I think we shouldn't remove this note.

@xelaris
Copy link
ContributorAuthor

@javiereguiluz That sounds good.

But there is also the other point. I think in the context of the chapter the reader never needs to create the AppBundle with this command, since it comes out of the box for all maintained versions, also before 2.6. At least the statement is not totally right. Maybe it should be changed to something like:

If the AppBundle is not present yet, you can generate it with the following command:

What do you think?

@javiereguiluz
Copy link
Member

@xelaris you are right. But that note is intended for people using any Symfony version which doesn't include the AppBundle, which are most of past versions.

Some people may want to refactor their application to use the new set of best practices, and the first step would be to create the AppBundle.

@xelarisxelaris changed the titleRemove tip for creating a new AppBundleClarify tip for creating a new AppBundleDec 29, 2014
@xelaris
Copy link
ContributorAuthor

@javiereguiluz I understand the need for this tip, when people want refactor their application. I have changed my commit. IMO this would make it more clear that you can have a version before 2.6 and do not need to (and can not) execute the command, since the AppBundle could be already there.

If you are using Symfony 2.6 or a newer version, the ``AppBundle`` bundle
is already generated for you. If you are using an older Symfony version,
you can generate it by hand executing this command:
If you are using a Symfony version, which does not come with a pregenerated
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of "If your Symfony installation doesn't come with a pregeneratedAppBundle, you can generate it by hand executing this command:"?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sounds better, going to change...

@@ -152,8 +152,7 @@ that follows these best practices:

.. tip::

If you are using Symfony 2.6 or a newer version, the ``AppBundle`` bundle
is already generated for you. If you are using an older Symfony version,
If your Symfony installation doesn't come with a pregenerated ``AppBundle``,
Copy link
Member

Choose a reason for hiding this comment

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

I think the word "pregenerated" doesn't exist. Can we use "pre-generated" or "previously generated" instead?

@javiereguiluz
Copy link
Member

@xelaris I like your suggestion, but if you read the new phrase carefully:

If your Symfony installation doesn't come with a pregenerated AppBundle ...

The only thing I don't like is that it exposes a "problem" without explaining the reason. Why doesn't my app include an AppBundle? Have I done something wrong? Have I downloaded the wrong Symfony version? Do I have to update my Symfony version before refactoring its code?

@xelaris
Copy link
ContributorAuthor

What do you think about

If you are using an existing Symfony installation, which doesn't come with a pre-generated ``AppBundle``, you can generate it by hand executing this command:

where the wordexisting explains the reason?

@javiereguiluz
Copy link
Member

@xelaris it's better ... but there are some missing reasons: perhaps you use the Symfony Installer to install a specific version (e.g. 2.3.23) that didn't include the AppBundle yet).

@xelaris
Copy link
ContributorAuthor

@javiereguiluz That's not true. A new project created withsymfony new myapp 2.3.23 includes the AppBundle. That is actually the main reason for this PR ;-). However it is not included in version 2.3.22 and lower, but I think, nobody would create a new project based on 2.3.22.

@xabbuh
Copy link
Member

A user could only get Symfony without the AppBundle using the installer if they did it before the pull request in the standard edition including the AppBundle was merged. Though this has been done a few weeks ago when the installier still was in an early state, right? So, I don't think that this really is an issue we need to take into account here.

@wouterj
Copy link
Member

I think we don't have to go into detail here. The best practices state that you create it usingsymfony new XXX, which will always give you the AppBundle. If you don't have it, for any reason, we show them how to create one. Carefully detailing about why one doesn't have it (one has an old installation, one installed an older version, one simply removed it because you thought you didn't need it, etc) will only move away from the essence of this tip imo.

And btw, this tip has to be as short as possible, as it's only important for a small group of people.

@javiereguiluz
Copy link
Member

@xabbuh@wouterj your comments made me change my mind. I agree with@xelaris proposal :)

@xelaris
Copy link
ContributorAuthor

I have changed the commit to the latest proposal (incl.pre-generated as@xabbuh pointed out), since it seems to be the agreement. Let me know if you prefer the previous shorter one.

@wouterj
Copy link
Member

I would prefer it without "existing", as it only adds confusion imo (and it makes it a bit harder to read).

The previous description was not quite right, since an AppBundle canalso be there in versions before 2.6.
@xelaris
Copy link
ContributorAuthor

Here is another change. I also prefer this shorter, generic description.

@xabbuh
Copy link
Member

👍 I like it.

@weaverryan
Copy link
Member

I like it too guys, a lot :). Thanks for a well-thought-out PR@xelaris!

@weaverryanweaverryan merged commit7b9d583 intosymfony:2.3Jan 3, 2015
weaverryan added a commit that referenced this pull requestJan 3, 2015
This PR was merged into the 2.3 branch.Discussion----------Clarify tip for creating a new AppBundle| Q             | A| ------------- | ---| Doc fix?      | yes| New docs?     | no| Applies to    | all| Fixed tickets |The tip seems to be confusing, since the AppBundle is also generated with `symfony new blog 2.3.23` (and 2.5.8). In addition the mentioned `generate:bundle` command does not work in SF 2.3 SE, since it stops with a `The namespace must contain a vendor namespace` error.Commits-------7b9d583 Clarify tip for creating a new AppBundle
@weaverryan
Copy link
Member

And@javiereguiluz, I just checked, we're (fortunately) not waiting onsensiolabs/SensioGeneratorBundle#312 for the "vendor namespace"generate:bundle issue to be better - I created a smaller PR that was merged in late Nov so that the error wasn't there for 2.6 (but 312 will actually make the rest of the command better). But, if you follow the standard composer.json in your project, only people in 2.5 or higher will have a new enough version of the generator bundle to get this change - I believe 2.3 users will still see the error. I don't think that's a huge deal - just an FYI for helping people :).

@xelarisxelaris deleted the patch-9 branchOctober 31, 2022 12:17
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@xelaris@javiereguiluz@xabbuh@wouterj@weaverryan

[8]ページ先頭

©2009-2025 Movatter.jp