Update to Sulu 2.0 - Githubissues

sulu / SuluRedirectBundle

Sulu bundle for managing redirects

MIT License

18 stars 19 forks source link

Update to Sulu 2.0 #32

Closed kleinkoerkamp closed 5 years ago

kleinkoerkamp commented 5 years ago

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
License	MIT

What's in this PR?

Update to work with Sulu 2.0

To Do

[ ] ~~Import via UI~~ #33
[x] UPGRADE.md

alexander-schranz commented 5 years ago

/cc @chirimoya and @danrot the toolbarAction on the list for Import I would see to be inside sulu/sulu where you can drop a file into it and call a specific action on the current resource.

kleinkoerkamp commented 5 years ago

@alexander-schranz thanks for the feedback, will dive into that. Regarding the import functionality in the UI i'm very curious if you guys have some quicktips how to implement it.

alexander-schranz commented 5 years ago

@danrot maybe you can give some tips here and maybe we have some design for an import drop in? /cc @chirimoya @kleinkoerkamp The js implementation will be in sulu/sulu repository here: https://github.com/sulu/sulu/tree/develop/src/Sulu/Bundle/AdminBundle/Resources/js/views/List/toolbarActions the toolbarAction need to be registered here and then can be used in the RedirectAdmin.php by the registered name.

kleinkoerkamp commented 5 years ago

@alexander-schranz changed most of it, except from the BC breaks where you asked @wachterjohannes his opinion.

Also do not yet really get the import drop in, should this be part of the Sulu core? /cc @danrot

alexander-schranz commented 5 years ago

The import can currently be seen as out of scope as it need first be implemented in the sulu/sulu repository: https://github.com/sulu/sulu/issues/4621 / https://github.com/sulu/SuluRedirectBundle/issues/33

alexander-schranz commented 5 years ago

We need to add the following SQL's to the UPGRADE.md to support utfmb4 which is default in sulu 2.0:

ALTER TABLE `re_redirect_routes` CHANGE `id` `id` VARCHAR(36) NOT NULL;
ALTER TABLE `re_redirect_routes` CHANGE `source` `source` VARCHAR(191) NOT NULL;

EDIT: Did add it in the last commit

danrot commented 5 years ago

@kleinkoerkamp Since we also have an export button for lists in the core already, I think it would also make sense to have the inverse operation. So yes, I think an import button being in Sulu core calling a certain Symfony action similar to what the import button does would make sense.

However, I also don't have any design for that. We were also discussing if clicking the button should simply open the operating systems file upload dialog. It probably would not make a lot of sense to upload multiple files at once, because it would just complicate things, for a probably not existent use case.

alexander-schranz commented 5 years ago

I'm merging this and ignoring the phpstan errors currently in the CI as they should be fixed with: https://github.com/sulu/sulu/pull/4655.

There are currently 2 things out of scope of this PR. Both are toolbarActions:

The Import Toolbar Action #33
The Enabled/Disable Toolbar Action #37

Both should be implemented in the js of sulu/core so other bundles can also use them.

/cc @chirimoya @danrot

alexander-schranz commented 5 years ago

@kleinkoerkamp thank you for your PR!