Skip to content
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

[GeneratorBundle] Kuma install command #1836

Merged
merged 5 commits into from Mar 22, 2019

Conversation

cv65kr
Copy link
Contributor

@cv65kr cv65kr commented Feb 18, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets no

Described in doc.

@cv65kr cv65kr force-pushed the feature/install branch 3 times, most recently from aab6822 to 585f637 Compare February 18, 2018 14:04
Copy link
Contributor

@sandergo90 sandergo90 left a comment

Choose a reason for hiding this comment

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

@cv65kr What's the difference with the setupProject.sh command that already exists?

use Sensio\Bundle\GeneratorBundle\Command\Helper\QuestionHelper;
use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand;
use Symfony\Component\Debug\Exception\ContextErrorException;

Copy link
Contributor

Choose a reason for hiding this comment

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

Add class docblock.

use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand;
use Symfony\Component\Debug\Exception\ContextErrorException;

class InstallCommand extends ContainerAwareCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

For new commands, we don't extend the ContainerAwareCommand anymore. It should work with service injections.

{
$this
->setName('kuma:install')
->setDescription('KunstmaanCMS installator.')
Copy link
Contributor

Choose a reason for hiding this comment

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

"installer"

@cv65kr
Copy link
Contributor Author

cv65kr commented Feb 19, 2018

@sandergo90, First of all, I wanted it to be consistent, without extra bash file. Secondly, we have less fields to fill, and also we can selected demosite install without bash file edit. If You think is not necessary, i will close this PR.

@sandergo90
Copy link
Contributor

@cv65kr Alright, maybe it's a good thing then!

@cv65kr
Copy link
Contributor Author

cv65kr commented Feb 19, 2018

@sandergo90, Changed according to your guidelines.

*/
public function __construct(ContainerInterface $container)
{
$this->container = $container;
Copy link
Contributor

Choose a reason for hiding this comment

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

Symfony 4 is deprecating using the container directly, in the bundles we also have several attempts going on to remove all places where we are injecting and using the database. I do not think its a good idea to add new code which uses the container now. Better just inject the dependencies you want.

$input->setOption('demosite', $demositeOption);
$input->setOption('bundle-name', strtr($namespace, ['\\Bundle\\' => '', '\\' => '']));

$dir = $input->getOption('dir') ?: dirname($this->container->getParameter('kernel.root_dir')) . '/src';
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need to inject the container to get this parameter. you can inject the parameter alone in the services.yml.

@Devolicious
Copy link
Contributor

Hi @cv65kr,

If you can change the injection of the container to just the %kernel.root_dir% parameter, like @Numkil said, it would be much nicer and we can merge this PR. If you have some time it would be much appreciated. (the merge conflict also please)

Thx!

@Devolicious Devolicious modified the milestones: 5.1.0, 5.2.0 Apr 13, 2018
@sandergo90
Copy link
Contributor

@cv65kr Do you have the time to finish this? Thanks!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @cv65kr, your PR passed all our requirements.

Thank you for contributing!

@Numkil Numkil force-pushed the feature/install branch 2 times, most recently from cab4f24 to 4f2a696 Compare January 24, 2019 10:34
@Numkil
Copy link
Contributor

Numkil commented Jan 24, 2019

@Devolicious @sandergo90

I updated changes I requested myself. Up to you guys if you still find it interesting to add this to the bundles or not.

@acrobat acrobat removed this from the 5.2.0 milestone Feb 1, 2019
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @cv65kr, your PR needs some changes

  • This PR seems to need a milestone of a minor release.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @cv65kr, your PR passed all our requirements.

Thank you for contributing!

@Devolicious Devolicious changed the title [WIP][GeneratorBundle] Kuma install command [GeneratorBundle] Kuma install command Mar 22, 2019
@Devolicious Devolicious merged commit bc35919 into Kunstmaan:master Mar 22, 2019
Devolicious added a commit that referenced this pull request Apr 18, 2019
* master: (406 commits)
  [NodeBundle] Allow entity as creator (#2429)
  [AllBundle] Separate bundle phpunit config and cleanup old files (#2419)
  check if db_driver has been configured as it cannot be overwritten (#2425)
  [AdminBundle] dynamic breakpoint for top navbar in admin area (#2375)
  [AdminBundle] update Bootstrap v3.3 -> v3.4 (#2424)
  [Documentation] KunstmaanCMS Installer documentation (#2418)
  [AdminBundle] fix typo in filename of wordcount (#2423)
  [FormBundle] In newer versions of symfony the @templating service is changed to the delegatingengine instead of the twigengine (#2422)
  [AllBundles] Replace deprecated getRootDir calls (#2421)
  [NodeBundle] Remove old/incorrect doc about lock command (#2420)
  fix null check (#2417)
  [GeneratorBundle] Added config generator + improved install command (#2414)
  [NodeBundle] Import commands.yml to register command in sf4 (#2415)
  [NodeBundle] Fix for missing validation of url/email link chooser (#2413)
  [AllBundles] added link script for development (#2412)
  [DashboardBundle] Mark google analytics helper service as public (#2410)
  [NodeBundle] Start page reorder weight at 1 instead of 0 (#2411)
  [TranslatorBundle] csv export creates garbage on special signs (#2409)
  [NodeSearchBundle] fix #2380 childs of structurenodes never get indexed (#2395)
  [GeneratorBundle] Kuma install command (#1836)
  ...
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.

None yet

6 participants