Skip to content
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

Add configuration for experimental content bundle storage #553

Merged
merged 7 commits into from
Mar 12, 2021
Merged

Add configuration for experimental content bundle storage #553

merged 7 commits into from
Mar 12, 2021

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Mar 10, 2021 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets
Related issues/PRs #430, #545
License MIT

What's in this PR?

Add alternative content bundle storage for articles.

Why?

Avoid requirement to elasticsearch and the need of knowledge about the phpcr.

Example Usage

sulu_article:
     article:
           storage: 'experimental'

@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch 3 times, most recently from 5b516fb to c137268 Compare March 11, 2021 13:23
@niklasnatter niklasnatter mentioned this pull request Mar 11, 2021
Closed
1 task
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from c137268 to 2f80a90 Compare March 11, 2021 14:33
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from 2f80a90 to 4508a9f Compare March 11, 2021 14:47
@alexander-schranz alexander-schranz changed the title WIP: Add alternative content bundle storage for articles Add configuration for experimental content bundle storage Mar 11, 2021
@alexander-schranz alexander-schranz added the Feature New functionality not yet included label Mar 11, 2021
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from bdf1a69 to 35fde55 Compare March 11, 2021 15:09
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from e716e10 to a3aa3e4 Compare March 11, 2021 16:03
@alexander-schranz alexander-schranz marked this pull request as ready for review March 11, 2021 16:13
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from 75e7ccc to ce6d9b8 Compare March 11, 2021 16:35
niklasnatter
niklasnatter reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

looks quite good 🙂

DependencyInjection/SuluArticleExtension.php Outdated
ArticleInterface::class => [
'generator' => 'schema',
'options' => [
'route_schema' => '/{object["title"]}',
Copy link
Contributor

Choose a reason for hiding this comment

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

would use implode("-", object) to match page behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

we should this then change also in the ExampleTestBundle in SuluContentBundle

DependencyInjection/SuluArticleExtension.php Outdated
$isPHPCRStorage = Configuration::ARTICLE_STORAGE_PHPCR === $storage;
$isExperimentalStorage = Configuration::ARTICLE_STORAGE_EXPERIMENTAL === $storage;

if ($isExperimentalStorage && $container->hasExtension('doctrine')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about creating a prependPHPCRStorage and prependExperimentalStorage method instead of adding all the conditions in this method? think that would be a bit easier to read

Copy link
Member Author

Choose a reason for hiding this comment

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

is fine for me

DependencyInjection/SuluArticleExtension.php Outdated
$isPHPCRStorage = Configuration::ARTICLE_STORAGE_PHPCR === $storage;
$isExperimentalStorage = Configuration::ARTICLE_STORAGE_EXPERIMENTAL === $storage;

if ($isPHPCRStorage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above - think i would add a loadPHPCRStorage and loadExperimentalStorage

Copy link
Member Author

Choose a reason for hiding this comment

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

done

SuluArticleBundle.php Outdated
{
$interfaces = [];

if (Configuration::ARTICLE_STORAGE_EXPERIMENTAL === $container->getParameter('sulu_article.article_storage')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would probably add two methods here too 😇

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 created methods to get the specific compilerpasses

Tests/Application/config/config_experimental.yml Outdated
@@ -0,0 +1,4 @@
sulu_article:
Copy link
Contributor

Choose a reason for hiding this comment

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

would call the file config_experimental_storage

Copy link
Member Author

Choose a reason for hiding this comment

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

done the call will be as mention futher down then.

Tests/Application/config/config_phpcr.yml Outdated
@@ -0,0 +1,27 @@
sulu_route:
Copy link
Contributor

Choose a reason for hiding this comment

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

would call the file config_phpcr_storage

Copy link
Member Author

Choose a reason for hiding this comment

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

done the call will be as mention futher down then.

Tests/Application/Kernel.php

public function __construct(string $environment, bool $debug, string $suluContext = SuluKernel::CONTEXT_ADMIN)
{
$environmentParts = explode('_', $environment, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used anywhere yet, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

not yet but if we begin to write functional tests then I need to start the client or boot the kernel with the new environment.

$this->client = $this->createAuthenticatedClient(['environment' => 'test_experimental']);

or if we go with your suggestion it would be:

$this->client = $this->createAuthenticatedClient(['environment' => 'test_experimental_storage']);

@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch 2 times, most recently from a796d8a to cb701cb Compare March 12, 2021 10:59
@alexander-schranz alexander-schranz force-pushed the feature/add-content-bundle-storage branch from cb701cb to 3fcc381 Compare March 12, 2021 11:03
@alexander-schranz alexander-schranz mentioned this pull request Mar 12, 2021
Merged
niklasnatter
niklasnatter reviewed Mar 12, 2021
View reviewed changes
Tests/Application/config/config_experimental_storage.yml
sulu_article:
article:
storage: 'experimental'
default_main_webspace: 'sulu_io'
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this is needed? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

currently not will remove it in #554

@niklasnatter niklasnatter merged commit 93ec60e into sulu:2.x Mar 12, 2021
@alexander-schranz alexander-schranz deleted the feature/add-content-bundle-storage branch March 12, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niklasnatter niklasnatter niklasnatter left review comments

Assignees

@alexander-schranz alexander-schranz

@niklasnatter niklasnatter

Labels
Feature New functionality not yet included
Projects
None yet
Milestone
No milestone
Development

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