Update to Sulu 2.0 #32
Conversation
There was a problem hiding this comment.
@kleinkoerkamp first a big thank you for doing this ugprade 👍 ! Some things we need to discuss with the other core team members but most of the things I commented should be easily fixable.
|
/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. |
|
@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. |
|
@danrot maybe you can give some tips here and maybe we have some design for an import drop in? /cc @chirimoya |
|
@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 |
There was a problem hiding this comment.
@kleinkoerkamp Can we change the Manager so it avoids currently the BC Break.
|
The import can currently be seen as out of scope as it need first be implemented in the sulu/sulu repository: sulu/sulu#4621 / #33 |
…to enhancement/sulu-2.0-upgrade
|
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 |
| @@ -5,23 +5,16 @@ | |||
| <services> | |||
| <!-- sulu-admin --> | |||
| <service id="sulu_redirect.admin" class="Sulu\Bundle\RedirectBundle\Admin\RedirectAdmin" public="false"> | |||
| <argument type="service" id="sulu_admin.route_factory"/> | |||
There was a problem hiding this comment.
@alexander-schranz Still not sure if this is a good idea. If the route builder factory is overridden by just changing the alias (which I would consider a valid thing to do) this class would still use the core service, instead of the overridden one.
Are there any real reasons for not doing this, besides the pure fact that we should not use FQCN?
There was a problem hiding this comment.
We should definitly following best practices and if somebody want to overwrite it for all bundles it should overwrite it by using the service id and not the FQCN.
There was a problem hiding this comment.
Where is written that this is best practice? I only know about the part of the symfony documentation saying that bundles should not rely on autowiring (which we don't do here), and it says nothing about how services should be named.
There was a problem hiding this comment.
If the bundle defines services, they must be prefixed with the bundle alias. For example, AcmeBlogBundle services must be prefixed with acme_blog.
https://symfony.com/doc/current/bundles/best_practices.html#services
|
@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. |
|
I'm merging this and ignoring the phpstan errors currently in the CI as they should be fixed with: sulu/sulu#4655. There are currently 2 things out of scope of this PR. Both are toolbarActions:
Both should be implemented in the js of sulu/core so other bundles can also use them. /cc @chirimoya @danrot |
|
@kleinkoerkamp thank you for your PR! |
What's in this PR?
Update to work with Sulu 2.0
To Do
Import via UIAdd import toolbar action #33