Add toolbar action to copy article #610
Conversation
alexander-schranz
changed the title
|
@alexander-schranz PR has been updated, I also added a basic test. |
| if (\count($locales) > 1) { | ||
| $editDropdownToolbarActions[] = new ToolbarAction('sulu_admin.copy_locale'); | ||
| } | ||
|
|
||
| $formToolbarActionsWithType[] = new DropdownToolbarAction( |
There was a problem hiding this comment.
I wasn't able to figure out the meaning of formToolbarActionsWithType and formToolbarActionsWithoutType, but so far it looks like this is the correct place.
| @@ -528,7 +528,7 @@ public function postTriggerAction(string $id, Request $request): Response | |||
| { | |||
| // extract parameter | |||
| $action = $this->getRequestParameter($request, 'action', true); | |||
| $locale = $this->getRequestParameter($request, 'locale', true); | |||
| $locale = $this->getRequestParameter($request, 'locale', false); | |||
There was a problem hiding this comment.
I had to change this to false, because the request does not provide any locale and it's working fine without locale. However, not sure if any other action requires the locale - in this case we would need to split the logic.
rogamoore
force-pushed
the
feature-copy-article
branch
from
695f139
to
02e90f6
Compare
|
Just a quick note: I will be away for three weeks now so I won't be able to respond to feedback. |
|
I'm back, so if there is anything I can do to bring this forward please let me know. |
alexander-schranz
force-pushed
the
feature-copy-article
branch
from
3e0e915
to
f9fb2cc
Compare
|
I repushed your branch. To retrigger a changed target branch pull request you need a new commit or change the commit you already pushed I mostly doing that via: git commit --amend --date="now" # replace existing commit with anew data
git push myremote mybranch --force # hard push as we replaced the commitIt seems like some tests are failing now on lower PHP versions. Can you have a look at them why they are failing? |
|
@alexander-schranz Thank you! I had a look - the tests are failing because as mentioned in the PR description right now it only works with Sulu >=2.6. We need this change https://github.com/sulu/sulu/pull/6859/files#diff-579b061b25841f1a028b4adc7b447e987e298306a441dd13346d08de3ec2d5ba to be backported to earlier Sulu versions (which I think shouldn't be a BC break) |
|
@rogamoore would make sense to me to backport that line to 2.4 and 2.5. Do you have time to target that today, then I could add it to the next release this week? |
|
@alexander-schranz Sure, I will take care of it this afternoon |
|
See sulu/sulu#7007 |
|
The release is tagged so we can update the composer.json with |
~2.4.11 || ^2.5.7 because sulu/sulu#7007 is needed
|
Tests are now green 🎉 |
|
Just to confirm, there is nothing missing from my side, right? |
|
Thx for pinging, I just wanted to check it out and test it manually if all works like expected. Sorry for the late response here, I tried it out now and did run into the following issue:
It could be related to that I have 2 languages, else all is like the default sulu installation with article bundle based on this branch 🤔 : <localizations>
<!-- See: http://docs.sulu.io/en/latest/book/localization.html how to add new localizations -->
<localization language="en" default="true"/>
<localization language="de"/>
</localizations>Do we maybe need to use a fallback locale if there is no locale provided by the copy toolbar action or is it maybe required that the copy toolbar action also provides the current locale, the pages copy action seems currently sending a I pushed my Repo here: https://github.com/alexander-schranz/reproducer-article-copy-error |
|
It seems to be related if there is route mapping defined for the ArticleDocument and live generation is used 🤔 |
|
Thanks for testing! Sounds a bit like the problem described here: #538 |
|
You are right with: sulu_route:
mappings:
Sulu\Bundle\ArticleBundle\Document\ArticleDocument:
generator: schema
options:
route_schema: '/articles/{is_array(object) ? implode("-", object) : object.getTitle()}'it works. We should document that in the UPGRADE.md. But maybe additional change we require to avoid running into that problem, will require to check that with @sulu/core-team. |
|
If the required changes are not too complicated, I could try to work on a PR. |
|
@rogamoore I debugged a little bit and prepared some changes in the sulu core: sulu/sulu#7041 do you maybe want to test if this now works in the different cases? |
|
@alexander-schranz Sorry, I remember seeing the notification but somehow then forgot about it. |
|
@rogamoore thank you! |
|
Thanks a lot! Can't wait for the release 👍 |
What's in this PR?
This adds the possibility to copy an existing article. It's similar to the "Copy Form" (sulu/SuluFormBundle#316) and "Copy Snippet" (sulu/sulu#6859) features.
Needs this change in
sulu/sulu: https://github.com/sulu/sulu/pull/6859/files#diff-579b061b25841f1a028b4adc7b447e987e298306a441dd13346d08de3ec2d5ba, which is currently only in 2.6 - maybe it can be backported to allow the feature to work with Sulu >=2.4.Please see also #538 for related discussion.