Skip to content

Conversation

erickloss
Copy link
Collaborator

No description provided.

Sebobo and others added 30 commits January 19, 2023 14:51
With this change the tailwind styles are only applied to the app container of the decoupled content store
This stops having to go through all individual worker outputs
Each document needs to be checked whether it’s still connected to the page tree though.
Only upload script once and reuse script and redis instance
BUGFIX: Add html escaping to stderr and stdout of job logs
Sebobo and others added 8 commits December 12, 2023 16:03
@erickloss erickloss requested review from Sebobo and JamesAlias April 23, 2025 10:06
Copy link
Contributor

@skurfuerst skurfuerst left a comment

Choose a reason for hiding this comment

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

Hey everybody, specifically @Sebobo and @erickloss ,

thanks for your contributions ❤️ Sorry I only now saw this pull request, please ping me next time in Slack :)

I am currently working on a bigger overhaul of the Decoupled ContentStore package, which will support other rendering targets than the default Fusion rendering as well. In the course of doing so, I have reviewed this change, and would like to discuss a few details. Overally, the code looks great ❤️ ❤️ ❤️

I'd love to make this a part of version 2 (my changes will be a bit breaking config-wise, but easy to adjust).

All the best,
Sebastian
PS: I am on holidays the next two weeks, so we'll find a spot for discussing this afterwards :)

use Neos\Flow\Annotations as Flow;

/**
* We usually rely on prunner to ensure that only one build is running at any given time.
* We usually rely on prunner to ensure that only one build per workspace is running at any given time.
Copy link
Contributor

Choose a reason for hiding this comment

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

new feature (for changelog / upgrade notes)

// Remove additional JSON data from log line
$line = preg_replace("/\{.*}/", '', $line);

// Add highlighting for log levels
Copy link
Contributor

Choose a reason for hiding this comment

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

new feature (for changelog / upgrade notes)

@@ -19,6 +16,11 @@ class IncrementalContentReleaseHandler
*/
protected $contentReleaseManager;

/**
* @Flow\InjectConfiguration("startIncrementalReleaseOnWorkspacePublish")
Copy link
Contributor

Choose a reason for hiding this comment

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

new feature (for changelog / upgrade notes)

{
return [
'contextPath' => $this->contextPath,
'nodeIdentifier' => $this->nodeIdentifier,
'dimensions' => $this->getDimensionsFromContextPath(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why dimensions extra here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure anymore. Probably unused.

@@ -33,6 +33,12 @@ class NodeContextCombinator
*/
protected $contextFactory;

/**
* @Flow\InjectConfiguration(path="nodeRendering.recurseHiddenContent", package="Flowpack.DecoupledContentStore")
Copy link
Contributor

Choose a reason for hiding this comment

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

new feature (for changelog / upgrade notes)


$contentReleaseLogger->debug('Registering node for publishing', [
'node' => $contextPath
if ($nodeToEnumerate->isHidden()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you wanted to allow hidden nodes as well?

Copy link
Member

Choose a reason for hiding this comment

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

We allow hidden content (due to project specifics), but not hidden documents.

}

public static function fromEnumeratedNode(EnumeratedNode $enumeratedNode)
{
return new self($enumeratedNode->getNodeIdentifier(), $enumeratedNode->getDimensionsFromContextPath(), $enumeratedNode->getArguments());
return new self($enumeratedNode->getNodeIdentifier(), $enumeratedNode->getDimensionsFromContextPath(), $enumeratedNode->getWorkspaceNameFromContextPath(), $enumeratedNode->getArguments());
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this change do anything? (modifications in this class)

Copy link
Member

Choose a reason for hiding this comment

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

This was not fully finished, see TODO below. I think we were lucky so far, that we didn't have collisions due to this.

'The cache backend for "Neos_Fusion_Content" must be an OptimizedRedisCacheBackend, but is ' . get_class(
$backend
), 1622570000
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is only code refactoring, no functional change?

Copy link
Member

Choose a reason for hiding this comment

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

No it was an optimisation Only upload script once and reuse script and redis instance

@@ -88,7 +87,7 @@ public static function fromJsonString($metadataEncoded, ContentReleaseIdentifier
isset($tmp['manualTransferJobIds']) ? array_map(function (string $item) {
return PrunnerJobId::fromString($item);
}, json_decode($tmp['manualTransferJobIds'])) : [],
$tmp['workspace'] ?? 'live',
$tmp['workspaceName'] ?? 'live',
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO check if this is correct

Copy link
Member

Choose a reason for hiding this comment

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

Yes, see the jsonSerialize method of ContentReleaseMetadata

@@ -36,6 +36,7 @@ pipelines:
script:
- ./flow contentReleasePrepare:createContentRelease {{ .contentReleaseId }} {{ .__jobID }} --workspaceName {{ .workspaceName }} --accountId {{ .accountId }}
- ./flow contentReleasePrepare:ensureAllOtherInProgressContentReleasesWillBeTerminated {{ .contentReleaseId }}
- ./flow contentReleasePrepare:flushContentCacheIfRequired {{ .contentReleaseId }} --flushContentCache={{ .flushContentCache }}
Copy link
Contributor

Choose a reason for hiding this comment

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

BREAKING, so needs tobe added to own pipelines.yaml when updating to 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants