Skip to content
This repository has been archived by the owner on Feb 4, 2023. It is now read-only.

Pass the container to the Factory so datatables and columns can be a service #784

Closed
wants to merge 12 commits into from
Closed

Conversation

mwansinck
Copy link
Contributor

In our custom columns we needed the Symfony Translator, which wasn't passed through the constructor.
By making the columns (and the datatables) a service, dependency injection becomes available.

Also added some missing composer requirements to require-dev for better code completion.

@Seb33300
Copy link
Collaborator

Seb33300 commented Aug 8, 2018

Can you give more explanations?

This makes able to call $this->container->get('...') to access the service we need in Datatables and Columns classes?

@mwansinck
Copy link
Contributor Author

$this->container->get('...') is only possible when the service is public.
Since Symfony4 services are private by default and so to use services in this classes is through making them services as well and inject the needed services with Dependency Injection.

mwansinck and others added 4 commits February 4, 2019 14:17
# Conflicts:
#	Datatable/AbstractDatatable.php
# Conflicts:
#	Datatable/AbstractDatatable.php
#	Datatable/Column/ColumnBuilder.php
@stephanvierkant
Copy link
Collaborator

@darylholling Please take a look at #903. I think that's a better way of having columns as a service. What do you think?

@mwansinck
Copy link
Contributor Author

@darylholling Please take a look at #903. I think that's a better way of having columns as a service. What do you think?

Hello @stephanvierkant, I think you are confusing my PR (#784) and the one of my college @darylholling (#904) :-) The approach of #903 seems fine to me, you may decline this PR. As soon as #904 has been released we can stop using fork..

Also I see this PR is based on the master branche of our fork, causing later improvements of us to be included in this PR. So another reason to use #903 instead of this one.

@stephanvierkant
Copy link
Collaborator

Closed in favour of #903. Thanks for your contribution nevertheless!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants