Skip to content

Type changes round4 #7926

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

Merged
merged 2 commits into from
May 27, 2017
Merged

Type changes round4 #7926

merged 2 commits into from
May 27, 2017

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented May 20, 2017

More "types" changes: using type-hinting instead of $container->get().

This should be my last round of big changes. But, I have not yet updated the following directories... so there is still more work. It turns out, the docs are HUGE :).

  • components
  • create_framework
  • reference (this is now WIP)

Thanks!

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Ryan, thanks for another fantastic contribution. You removed three times more lines than you added/changed, so I couldn't be happier!!

Cheers!

be :ref:`defined as private <container-private-services>`.
be :ref:`defined as private <container-private-services>`. For public services,
:ref:`aliases should be created <service-autowiring-alias>` from the interface/class
to the service id. For example, in MonlogBundle, an alias is created from
Copy link
Member

Choose a reason for hiding this comment

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

MonlogBundle -> MonologBundle

While making life easier, this has some limitations:
If you're using the :ref:`default services.yml configuration <service-container-services-load-example>`,
your command classes are already registered as services. Great! This is the recommended
setup, but it's not required. Symfony also looks in the ``Command`` directory of
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 we always adda a trailing slash to dir names to not confuse them with files. Command -> Command/


To solve these problems, you can register your command as a service and tag it
with ``console.command``:
You can also manually register your command as a service by configure the service
Copy link
Member

Choose a reason for hiding this comment

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

by configure-> by configuring ?

)
;
->setName('app:sunshine')
->setDescription('Hello PhpStorm');
Copy link
Member

Choose a reason for hiding this comment

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

Is it OK to use this description?

Be careful not to actually do any work in ``configure`` (e.g. make database
queries), as your code will be run, even if you're using the console to
execute a different command.
You *do* have access to services in ``configure()``. However, try to avoid doing
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to upgrade this paragraph when lazy console commands are merged in Symfony 4. There's a PR for that: symfony/symfony#22734

{
return $this->container->get('templating')->renderResponse($view, $parameters, $response);
}
The base `Controller class source code`_ is a great way to see how to performance
Copy link
Member

Choose a reason for hiding this comment

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

how to performance -> how to perform


public function __construct(EngineInterface $templating)
public function __construct(\Twig_Environment $twig)
Copy link
Member

Choose a reason for hiding this comment

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

This is one of those cases where using the injection of services look weird. The Twig_Environment name is really bad. I don't want Twig's environment (whatever that may be), I want the Twig service to render Twig templates. But, Tiwg Environment is not only the environment, is also the main service you are looking for And why don't you call it simply Twig? :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree... and - the new system makes it more obvious when a class is poorly-named in a library we use. Before, we could "hide" it behind our Symfony-specific service ids. I think it's something that needs to be improved in the underlying libraries (sometimes, like when the underlying classes are from Symfony - I'm thinking about security) - we can make that change ourselves :). For Twig, it might be interesting to introduce a new Twig or TwigRenderer class that only has a sub-set of the public functions that Twig_Environment has.

return $this->templating->renderResponse(
'AppBundle:Hello:index.html.twig',
$content = $this->twig->renderResponse(
'hellp/index.html.twig',
Copy link
Member

Choose a reason for hiding this comment

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

hellp/index.html.twig -> hello/index.html.twig


return new StreamedResponse($callback);
If you want to know what type-hints to use for each service, see the
``getSubscribedEvents`` in `AbstractController`_.
Copy link
Member

Choose a reason for hiding this comment

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

see the getSubscribedEvents in -> see the getSubscribedEvents() method in

old service should be kept around to be able to reference it in the
new one. This configuration replaces ``app.mailer`` with a new one, but keeps
a reference of the old one as ``app.decorating_mailer.inner``:
you might want to decorate the old service instead: keeping the old service so
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit weird to use two colons in this phrase. You could use a comma or a semicolon after instead

@weaverryan weaverryan merged commit c8e1c96 into symfony:3.3 May 27, 2017
weaverryan added a commit that referenced this pull request May 27, 2017
This PR was squashed before being merged into the 3.3 branch (closes #7926).

Discussion
----------

Type changes round4

More "types" changes: using type-hinting instead of `$container->get()`.

This should be my last round of big changes. But, I have not yet updated the following directories... so there is still more work. It turns out, the docs are HUGE :).

- components
- create_framework
- reference (this is now WIP)

Thanks!

Commits
-------

c8e1c96 Tweaks thanks to Javier's reviews
86ab47a more type changes
@weaverryan weaverryan deleted the type-changes-round4 branch May 27, 2017 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants