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 copy article functionality #538

Closed
alexandersch opened this issue Dec 18, 2020 · 11 comments · Fixed by #610
Closed

Add copy article functionality #538

alexandersch opened this issue Dec 18, 2020 · 11 comments · Fixed by #610
Labels
Feature New functionality not yet included

Comments

@alexandersch
Copy link

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

@alexander-schranz alexander-schranz added the Feature New functionality not yet included label Dec 21, 2020
@rogamoore
Copy link
Contributor

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:

Next steps?

  • Extend the CopyToolbarAction that was implemented in Implement CopyToolbarAction for Form view sulu#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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

Jelmer7 commented Sep 28, 2022

👋 Any update on this?

@rogamoore
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

@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).

$route = $this->conflictResolver->resolve($this->chainRouteGenerator->generate($localizedDocument));
        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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants