Skip to content

Move sulu core structure config to sulu admin config #7908

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 2, 2025
Merged

Move sulu core structure config to sulu admin config #7908

merged 2 commits into from
May 2, 2025

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Apr 22, 2025 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Fixed tickets fixes #
Related issues/PRs #
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Move sulu core structure config to sulu admin config.

Why?

We should get rid of structure specific config and make all kind of metadata based on Sulu Admin service nothinger else.

Example Usage

Current: https://docs.sulu.io/en/2.6/cookbook/override-dir-structure.html#overwrite-templates-configuration-files-path

Not sure yet. Open the PR to discuss this Option A:

sulu_admin:
    forms:
        - '%kernel.project_dir%/config/forms'
    lists:
        - '%kernel.project_dir%/config/lists'
    templates:
        default_type:
            page: default
        paths:
            page_skeleton: # we use a key already to make things overwrite able here
                path: '%kernel.project_dir%/config/templates/pages'
                type: page # not sure if we should go with pages now

alternative B:

sulu_admin:
    forms:
        - '%kernel.project_dir%/config/forms'
    lists:
        - '%kernel.project_dir%/config/lists'
    templates:
        page:  # better use resource key here?
            default_type: default
            paths:
                page_skeleton: '%kernel.project_dir%/config/templates/pages'

/cc @Prokyonn

This introduces a new parameter sulu_admin.templates.configuration which we can later use in the new loader:

bin/console debug:container --parameter sulu_admin.templates.configuration --format=json

ToDo

  • Write parameter
  • Parse XMLs (upcoming Pull Request)

@Prokyonn
Copy link
Member

Personally I would go with variant B, as this is more readable and easier to extend because you immediately see, which parameters are dependant on each other, but it will be a bigger BC break and more difficult to migrate

@alexander-schranz
Copy link
Member Author

Think the migration is also for variant B kind of fine in both cases they need modify a few keys and at the end the container is only valid if they give a valid parts in it.

Migration for Variant B would look like this:

-sulu_core:
+sulu_admin:
-    content:
+    templates:
-         structure:
+         pages:
             paths:
-                 page_projectA:
-                     path: '%kernel.project_dir%/config/templates/pages/projectA'
-                     type: page
+                 page_projectA: '%kernel.project_dir%/config/templates/pages/projectA'
-                 page_projectB:
-                     path: '%kernel.project_dir%/config/templates/pages/projectB'
-                     type: page
+                 page_projectB: '%kernel.project_dir%/config/templates/pages/projectB'

@alexander-schranz alexander-schranz force-pushed the enhancement/move-structure-config-to-admin-config branch 3 times, most recently from 206c683 to 13ee98e Compare May 2, 2025 07:08
@alexander-schranz alexander-schranz force-pushed the enhancement/move-structure-config-to-admin-config branch from 13ee98e to b33b3e3 Compare May 2, 2025 07:08
@alexander-schranz alexander-schranz marked this pull request as ready for review May 2, 2025 07:09
@alexander-schranz alexander-schranz added the Technical Debt Impacts code quality, no or just small impact on end developers and users label May 2, 2025
alexander-schranz
alexander-schranz commented May 2, 2025
View reviewed changes
src/Sulu/Bundle/AdminBundle/DependencyInjection/Configuration.php
->defaultValue(null)
->example('default')
->end()
->arrayNode('directories')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Prokyonn did go with directories like we already did for forms and lists

alexander-schranz
alexander-schranz commented May 2, 2025
View reviewed changes
packages/article/src/Infrastructure/Symfony/HttpKernel/SuluArticleBundle.php
@@ -274,6 +274,14 @@ public function prependExtension(ContainerConfigurator $container, ContainerBuil
// \dirname(__DIR__, 4) . '/config/forms',
],
],
'templates' => [
'articles' => [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did go with the resource key now

alexander-schranz
alexander-schranz commented May 2, 2025
View reviewed changes
src/Sulu/Bundle/AdminBundle/DependencyInjection/SuluAdminExtension.php
@@ -144,16 +144,17 @@ public function load(array $configs, ContainerBuilder $container): void
$configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);

$container->setParameter($this->getAlias() . '.name', $config['name']);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the getAlias() and used hardcoded value to make the parameters easier to find with searches

@alexander-schranz alexander-schranz merged commit 889d884 into sulu:3.0 May 2, 2025
9 checks passed
@alexander-schranz alexander-schranz deleted the enhancement/move-structure-config-to-admin-config branch May 2, 2025 07:17
Prokyonn
Prokyonn reviewed May 2, 2025
View reviewed changes
src/Sulu/Bundle/AdminBundle/DependencyInjection/SuluAdminExtension.php

$container->setParameter('sulu_admin.forms.directories', $config['forms']['directories'] ?? []);
$container->setParameter('sulu_admin.lists.directories', $config['lists']['directories'] ?? []);
$container->setParameter('sulu_admin.templates.configuration', $config['templates']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is some inconsistency with the parameter names

$config['collaboration']['enabled'] translates to sulu_admin.collaboration_enabled
but
$config['forms']['directories'] translates to sulu_admin.forms.directories

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really strange not sure why this is. I know that when 2.0 was build it was defined that service name should always be <bundle>.<service> and so no more as one dot should be inside them. Strange this wasn't done for the parameters here.

@alexander-schranz alexander-schranz mentioned this pull request Jun 2, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prokyonn Prokyonn Prokyonn approved these changes

Assignees
No one assigned
Labels
Technical Debt Impacts code quality, no or just small impact on end developers and users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexander-schranz @Prokyonn