sulu / SuluArticleBundle

Bundle for managing localized content-rich entities like blog-posts in the Sulu content management system
MIT License
52 stars 77 forks source link

Add toolbar action to copy article #610

Closed rogamoore closed 1 year ago

rogamoore commented 2 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #538
Related issues/PRs -
License MIT

What's in this PR?

This adds the possibility to copy an existing article. It's similar to the "Copy Form" (https://github.com/sulu/SuluFormBundle/pull/316) and "Copy Snippet" (https://github.com/sulu/sulu/pull/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.

rogamoore commented 1 year ago

@alexander-schranz PR has been updated, I also added a basic test.

rogamoore commented 1 year ago

Just a quick note: I will be away for three weeks now so I won't be able to respond to feedback.

rogamoore commented 1 year ago

I'm back, so if there is anything I can do to bring this forward please let me know.

alexander-schranz commented 1 year ago

@rogamoore

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 commit

It seems like some tests are failing now on lower PHP versions. Can you have a look at them why they are failing?

rogamoore commented 1 year ago

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

alexander-schranz commented 1 year ago

@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?

rogamoore commented 1 year ago

@alexander-schranz Sure, I will take care of it this afternoon

rogamoore commented 1 year ago

See https://github.com/sulu/sulu/pull/7007

alexander-schranz commented 1 year ago

The release is tagged so we can update the composer.json with "sulu/sulu": "~2.4.11 || ^2.5.7".

rogamoore commented 1 year ago

Tests are now green 🎉

rogamoore commented 1 year ago

Just to confirm, there is nothing missing from my side, right?

alexander-schranz commented 1 year ago

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:

Bildschirmfoto 2023-03-06 um 11 20 42

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 locale=.. parameter.

I pushed my Repo here: https://github.com/alexander-schranz/reproducer-article-copy-error

alexander-schranz commented 1 year ago

It seems to be related if there is route mapping defined for the ArticleDocument and live generation is used 🤔

rogamoore commented 1 year ago

Thanks for testing! Sounds a bit like the problem described here: https://github.com/sulu/SuluArticleBundle/issues/538

alexander-schranz commented 1 year ago

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.

rogamoore commented 1 year ago

If the required changes are not too complicated, I could try to work on a PR.

alexander-schranz commented 1 year ago

@rogamoore I debugged a little bit and prepared some changes in the sulu core: https://github.com/sulu/sulu/pull/7041 do you maybe want to test if this now works in the different cases?

rogamoore commented 1 year ago

@alexander-schranz Sorry, I remember seeing the notification but somehow then forgot about it. Looks promising, I will test it this afternoon!

alexander-schranz commented 1 year ago

@rogamoore thank you!

rogamoore commented 1 year ago

Thanks a lot! Can't wait for the release 👍