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

add ClonePagesCommand #514

Closed
wants to merge 3 commits into from
Closed

add ClonePagesCommand #514

wants to merge 3 commits into from

Conversation

koyaan
Copy link

@koyaan koyaan commented Jul 30, 2015

Never did get around to refactor this into services unfortunately

$blockClones = array();

// TODO: check if we are missing parents
$output->writeln('Cloning pages');
Copy link
Member

Choose a reason for hiding this comment

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

Bad indent.

@soullivaneuh
Copy link
Member

Can you explain a little bit your feature? Some sample would be great.

Also documentation need to be updated.

@soullivaneuh
Copy link
Member

@koyaan
Copy link
Author

koyaan commented Jul 30, 2015

@soullivaneuh doc is in the RFC, and this is pretty rough stuff still

@soullivaneuh
Copy link
Member

Didn't see the link, my bad.

CS need to be fixed. BTW, indent should be 4 spaces, not 2.

You can solve CS quickly with make cs command (make sur to get last stable version of php-cs-fixer).

https://travis-ci.org/sonata-project/SonataPageBundle/jobs/73343238#L369

* {@inheritdoc}
*/
public function configure() {
$this->setName( 'sonata:page:clone' );
Copy link
Member

Choose a reason for hiding this comment

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

No spaces between parenthesis.

*/
class ClonePagesCommand extends BaseCommand
{
/**
Copy link
Member

Choose a reason for hiding this comment

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

Bad indent.

$output->writeln(sprintf(' cloning block % 4s ', $block->getId()));
$newBlock = clone $block;
$newBlock->setPage($newPage);
$blockClones[ $block->getId() ] = $newBlock;
Copy link
Member

Choose a reason for hiding this comment

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

No spaces.

@soullivaneuh
Copy link
Member

Also a related test files should be written.

I'll let @rande manage this PR after code review.

$this->setName('sonata:page:clone');
$this->setDescription('Clone pages');
$this->addOption('sourcesite', null, InputOption::VALUE_REQUIRED, 'Source Site id', null);
$this->addOption('destsite', null, InputOption::VALUE_REQUIRED, 'Dest Site id', null);
Copy link
Member

Choose a reason for hiding this comment

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

IMO dest-site would be better

$source_site = $this->getSiteManager()->findBy(array('id' => $input->getOption('sourcesite')));
$dest_site = $this->getSiteManager()->findBy(array('id' => $input->getOption('destsite')));

$pages = $this->getPageManager()->findBy(array('site' => $source_site[0]));
Copy link
Member

Choose a reason for hiding this comment

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

would use findOneBy() as we can use $source_site variable directly instead of $source_site[0]

@OskarStark
Copy link
Member

@koyaan would you please apply our comments?

We would really like this feature merged 😄

@soullivaneuh
Copy link
Member

Tests for the new command should be added.

@core23
Copy link
Member

core23 commented Feb 11, 2016

Please keep on working on this 👍 @koyaan

@soullivaneuh
Copy link
Member

According to the new Sonata version management and next major release plan, this project has been refactored regarding branching and versioning.

If you see this message, your PR concerns a patch or a minor release and is not targeting the right branch.

So I'm closing this one, but don't see it as a refusal. If you think your work is still relevant and want to continue, feel free to reopen it on the right branch (e.g. the default one).

Regards.

@core23 core23 mentioned this pull request May 28, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants