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

4609 multiple content collection publishing #4616

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

nezaniel
Copy link
Member

This resolves #4609

Population of NodeAggregateIdsByNodePaths is now done properly, recursively and has its own test cases

Checklist

  • Code follows the PSR-12 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the 9.0 branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Comment on lines 103 to 107
public function completeForNodeOfType(NodeTypeName $nodeTypeName, NodeTypeManager $nodeTypeManager): self
{
return self::fromArray($this->createNodeAggregateIdsForNodeType($nodeTypeName, $nodeTypeManager))
->merge($this);
}
Copy link
Member

Choose a reason for hiding this comment

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

i really like this method or here :D and was thinking as well to put it here.

(originally it was hidden here

protected static function populateNodeAggregateIds(
NodeType $nodeType,
?NodeAggregateIdsByNodePaths $nodeAggregateIds,
NodePath $childPath = null
): NodeAggregateIdsByNodePaths {
if ($nodeAggregateIds === null) {
$nodeAggregateIds = NodeAggregateIdsByNodePaths::createEmpty();
}
// TODO: handle Multiple levels of autocreated child nodes
foreach ($nodeType->getAutoCreatedChildNodes() as $rawChildName => $childNodeType) {
$childName = NodeName::fromString($rawChildName);
$childPath = $childPath
? $childPath->appendPathSegment($childName)
: NodePath::fromString($childName->value);
if (!$nodeAggregateIds->getNodeAggregateId($childPath)) {
$nodeAggregateIds = $nodeAggregateIds->add(
$childPath,
NodeAggregateId::create()
);
}
}
return $nodeAggregateIds;
}
)

Putting it here exposed will be important for the node templates package as we need it there.

Though i would prefer if there also was a static factory for this so that i dont have to write

NodeAggregateIdsByNodePaths::createEmpty()->completeForNodeOfType(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

like this?

Copy link
Member

Choose a reason for hiding this comment

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

yes looks pretty practical - and one can easily find this method.

$nodeAggregateIdsByNodePaths = NodeAggregateIdsByNodePaths::createForNodeType($nodeTypeName, $nodeTypeManager);

ideally we should enrich also the doc comments for the both methods about the why one would want to do this. (And maybe also a little about the "what" - that it recursively generates node ids for all declared tethered paths.)

@nezaniel nezaniel requested a review from mhsdesign October 14, 2023 10:33
And I am in content stream "cs-identifier"

Then I expect a node identified by cs-identifier;sir-david-nodenborough;{} to exist in the content graph

Copy link
Member

@bwaidelich bwaidelich Oct 14, 2023

Choose a reason for hiding this comment

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

Bug => Test = ❤️

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

I took the liberty of adjusting the documentation of CreateNodeAggregateWithNode::withTetheredDescendantNodeAggregateIds so it reflects the main user-land purpose.

Is this fine with you?

@nezaniel nezaniel merged commit 15b8d31 into 9.0 Oct 18, 2023
5 checks passed
@nezaniel nezaniel deleted the 4609-multipleContentCollectionPublishing branch October 18, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

BUG: Documents with multiple content collections and initial content cannot be published
4 participants