Add copy article functionality

[alexandersch]

I would like to request a copy article feature. Would be a nice addition to the article bundle.

[rogamoore]

Hello everybody!

We would also be interested to have this feature and are happy to help making it happen.

We noticed that there is already some logic in the current version for copying articles so it looks like this feature was already there in an older version of the bundle: see https://github.com/sulu/SuluArticleBundle/blob/2.x/Controller/ArticleController.php#L565

PoC

With some hacks it's pretty easy to get it working:

• Add a toolbar action (using the same action as for the copy form feature) - see https://github.com/sulu/SuluArticleBundle/pull/610/files#diff-b70d69ce3c1966544518ae0ffa0b76390ad94b390ac0f749faceaaf71992841aR242
• The copy action expects a locale which the existing toolbar action does not provide, so for testing we need to hardcode it, see https://github.com/sulu/SuluArticleBundle/pull/610/files#diff-6bb92e406267424def35c1c8b3b9d0bcbc4e784b7dd15300af0dab93a7b94477R521
• Now we already have a copy action available:
• However, it still fails in two places:
  • https://github.com/sulu/sulu/blob/2.5/src/Sulu/Component/Content/Document/Subscriber/WebspaceSubscriber.php#L93 Here it fails in deleteUnavailableLocales when calling $webspace->getAllLocalizations()
• https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/RouteBundle/Document/Subscriber/RoutableSubscriber.php#L263 Here it fails in handleCopy when calling the chainRouteGenerator
• Skipping both problematic places by adding a return statement to these methods makes the feature work and creates a draft copy of an existing article.

Next steps?

• Extend the CopyToolbarAction that was implemented in https://github.com/sulu/sulu/pull/6475 to provide the locale when triggering the copy action in the controller
• Find out why the two subscribers are failing and what needs to be done to make it work. Honestly I have no idea at the moment how it could be fixed - so any hints are welcome.
• Anything else we missed?

So what do you think?

[rogamoore]

Hi, I was wondering if someone could give me a quick feedback. If there is interest I could start working on a PR.

[mario-fehr]

Hi, I was wondering if someone could give me a quick feedback. If there is interest I could start working on a PR.

@rogamoore It would be nice if you could contribute this feature to the bundle.

[Jelmer7]

👋 Any update on this?

[rogamoore]

@mario-fehr @Jelmer7 Please see the "Next steps?" section on my post. I need feedback from someone (Sulu Team) on the two subscribers that are failing. They are not part of the SuluArticleBundle, but of Sulu Core and I have not enough knowledge at the moment to fix this part.

[rogamoore]

@alexander-schranz Sorry to ping you, but as we have some time available to contribute to this feature, I would like to ask for a quick opinion/feedback of the Sulu team regarding the above questions. If it's a bad moment, just let me know, otherwise I think it could be a great addition to the next release.

[alexander-schranz]

@rogamoore I think the copy function should be very similar to the snippet copy function which was lately implemented here by @wachterjohannes: https://github.com/sulu/sulu/pull/6859/files. Let me know if you can get it based on https://github.com/sulu/sulu/pull/6859/files get it work also for articles.

The deleteUnavailableLocales sounds like something which can be skipped in copy where no locale exist.

The RoutableSubscriber sounds required. It is strange that it fails here and the documentInspector can not get all locales? Or what is the current stack trace here?

[rogamoore]

@alexander-schranz Thanks for your feedback! I've checked the PR you've linked (https://github.com/sulu/sulu/pull/6859/files) and my changes are pretty much the same (which I think is a good sign) with the difference that after copying the article it's not published immediately, but opened in draft mode.

The linked PR also fixes one of my problems mentioned above, where it fails in the WebspaceSubscriber.

So right now there is only one issue left (all other details can be discussed in the upcoming PR).

• The RoutableSubscriber calls the ChainRouteGenerator here: https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/RouteBundle/Document/Subscriber/RoutableSubscriber.php#L278

$route = $this->conflictResolver->resolve($this->chainRouteGenerator->generate($localizedDocument));

• Note, that it doesn't provide the second parameter of generate(), which is $path
• It then enters this if-Condition here: https://github.com/sulu/sulu/blob/0c564eb9b8df8a163dc6a4381c710b003f80641a/src/Sulu/Bundle/RouteBundle/Generator/ChainRouteGenerator.php#L52

        if (!$path) {
            $generator = $this->routeGenerators[$config['mapping']['generator']];
            $path = $generator->generate($entity, $config['mapping']['options']);
        }

• Here it fails with Warning: Undefined array key ""
• I dumped the $config variable at this point:

array:2 [
  "className" => "Sulu\Bundle\ArticleBundle\Document\ArticleDocument"
  "mapping" => array:3 [
    "resource_key" => "articles"
    "generator" => null
    "options" => []
  ]
]

• As you can see, generator is null, but will cast into "" when accessing the $this->routeGenerators array.
• Here is a dump of $this->routeGenerators:

array:1 [
  "null" => Sulu\Bundle\RouteBundle\Generator\NullRouteGenerator
]

• Here is the stack trace:

ErrorException:
Warning: Undefined array key ""

  at vendor/sulu/sulu/src/Sulu/Bundle/RouteBundle/Generator/ChainRouteGenerator.php:53
  at Sulu\Bundle\RouteBundle\Generator\ChainRouteGenerator->generate()
     (vendor/sulu/sulu/src/Sulu/Bundle/RouteBundle/Document/Subscriber/RoutableSubscriber.php:278)
  at Sulu\Bundle\RouteBundle\Document\Subscriber\RoutableSubscriber->handleCopy()
     (vendor/symfony/event-dispatcher/EventDispatcher.php:270)
  at Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}()
     (vendor/symfony/event-dispatcher/EventDispatcher.php:230)
  at Symfony\Component\EventDispatcher\EventDispatcher->callListeners()
     (vendor/symfony/event-dispatcher/EventDispatcher.php:59)
  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch()
     (vendor/sulu/sulu/src/Sulu/Component/DocumentManager/DocumentManager.php:85)
  at Sulu\Component\DocumentManager\DocumentManager->copy()
     (vendor/sulu/article-bundle/Controller/ArticleController.php:565)
  at Sulu\Bundle\ArticleBundle\Controller\ArticleController->postTriggerAction()
     (vendor/symfony/http-kernel/HttpKernel.php:163)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (vendor/symfony/http-kernel/HttpKernel.php:75)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (vendor/symfony/http-kernel/Kernel.php:202)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (public/index.php:77)                

So now this raises a few questions:

• When generator is null, should the NullRouteGenerator be called? If so, it looks like a bug regarding the way the array key is used.
• Should there be only a NullRouter in the chain? I would expect that it would also have ArticleRouteGeneratorByType, ArticleRouteGeneratorByTemplate and ArticlePageRouteGenerator in there when using the article bundle
• Should the generate method even be called without a $path?

[rogamoore]

So, I found that the reason why generator is null is because in my config there is no mapping defined:

# missing
sulu_route:
    mappings:
        Sulu\Bundle\ArticleBundle\Document\ArticleDocument:
            generator: schema
            options:
                route_schema: '/articles/{implode("-", object)}'

The reason is that I have configured the route schema per template as described here: https://github.com/sulu/SuluArticleBundle/blob/2.x/Resources/doc/routing.md#overwrite-route-schema-in-template

<property name="routePath" type="route" mandatory="true">
    <meta>
        <title>sulu_admin.url</title>
    </meta>
    <params>
        <param name="route_schema" value="/{translator.trans('article.routes.events')}/{implode('-', object)}"/>
        <param name="mode" value="leaf"/>
    </params>
    <tag name="sulu_article.article_route"/>
</property>

So this raises a new question: If the route schema is defined in the template, how to make sure it's also used when copying an article?

Meanwhile I will test the yaml config to see, if I can get past the error with this.

[rogamoore]

I can get past the error, when using this mapping (not the default from the docs):

sulu_route:
    mappings:
        Sulu\Bundle\ArticleBundle\Document\ArticleDocument:
            generator: schema
            options:
                route_schema: '/articles/{is_array(object) ? implode("-", object) : object.getTitle()}'

Without the default from the docs I'm getting

implode(): Argument #2 ($array) must be of type ?array, Sulu\\Bundle\\ArticleBundle\\Document\\ArticleDocument given

My next step will be preparing a PR, but I will need some help with this route generating stuff, because IMHO it should work with the default YAML config and also when using the route schema per template config.

[rogamoore]

I think this issue can be closed now that #610 has been merged - the feature will be available in the next release (2.5.0 I guess)