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.

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

Add versioning to dimension content #202

Draft
wants to merge 1 commit into
base: 0.8
Choose a base branch
from

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Oct 2, 2021

Build on top of: #190

To be discussed:

  • Optimize getPublishLocales on write?
  • Optimize getLatestVersion on write?
  • Versioning an own Interface (make getEffectiveDimensionAttributes and getDefaultDimensionAttributes more complicated is it worse?)
  • Is Restore version a workflow transition?
  • Should Version be an own Entity / Table extending from other entity? (complexity on overwrite entity (could be validated in compilerpass))

TODO:

  • UI Versioning list on settings
  • Restore Version

// +---+ +---+ | | restore | |
// restore publish | +----------------------+ |
// | publish |
// +--------------------------------------------------------------------+
Copy link
Member Author

@alexander-schranz alexander-schranz Oct 2, 2021

Choose a reason for hiding this comment

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

See whole Graphic

/cc @wachterjohannes not sure if even possible have a transition with the same name with different source and target state?

published     ----restore---> draft
unpublished   ----restore---> unpublished

Copy link
Member

Choose a reason for hiding this comment

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

Is Restore version a workflow transition?

Answer in my opinion - yes - it should be if possible

if even possible have a transition with the same name with different source and target state?

Yes it is for multiple sources - but as i know only to a single destination. So your example i dont think its possible (maybe with a guard or a listener which sets the destination dynamically by using the further state?)

@alexander-schranz alexander-schranz force-pushed the feature/versioning branch 4 times, most recently from c257000 to 5d06dd5 Compare October 2, 2021 17:21
@coveralls
Copy link

coveralls commented Oct 2, 2021

Pull Request Test Coverage Report for Build 1298710368

  • 155 of 155 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1291214054: 0.0%
Covered Lines: 2059
Relevant Lines: 2059

💛 - Coveralls

@alexander-schranz alexander-schranz force-pushed the feature/versioning branch 2 times, most recently from ce78edb to fbb4d74 Compare October 2, 2021 17:34
@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Bundle label Oct 2, 2021
Copy link
Member

@wachterjohannes wachterjohannes left a comment

Choose a reason for hiding this comment

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

am i right when i say the restore itself is not build until now? only the save to a new version during publishing?

@@ -68,6 +77,21 @@ public function onPublish(TransitionEvent $transitionEvent): void

$dimensionAttributes['stage'] = DimensionContentInterface::STAGE_LIVE;

// create new version
// TODO optimize latest version and publish locales on write process to avoid loading them here?
Copy link
Member

Choose a reason for hiding this comment

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

👍 we should optimize this information

$contentRichEntity,
\array_merge($dimensionAttributes, ['locale' => $publishLocale]),
$contentRichEntity,
\array_merge($dimensionAttributes, ['locale' => $publishLocale, 'version' => $version])
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice solution with the version as attribute

Content/Domain/Model/DimensionContentCollection.php Outdated Show resolved Hide resolved
Content/Domain/Model/DimensionContentTrait.php Outdated Show resolved Hide resolved
@wachterjohannes
Copy link
Member

Optimize getPublishLocales on write?

Yes

Optimize getLatestVersion on write?

Yes

Versioning an own Interface (make getEffectiveDimensionAttributes and getDefaultDimensionAttributes more complicated is it worse?)

Would say no - as long as the implementation of the custom entity wont get to complex (doesnt look like as the moment) at the end this should be a basic and integrated feature for all entities.

Is Restore version a workflow transition?

Already answered in the review above.

Should Version be an own Entity / Table extending from other entity? (complexity on overwrite entity (could be validated in compilerpass))

Would say no - But we should discuss that with @chirimoya because of the problem that the table can get really big (bigger than it is already). But that problem we also have with the current phpcr implementation (at least with doctrine dbal storage)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in Bundle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants