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

IBX-5385: Add option content-type to reindex command #370

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,23 +128,28 @@ protected function configure()
'since',
null,
InputOption::VALUE_OPTIONAL,
'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids" or "subtree"'
'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree" or "content-type"'
Copy link
Member

Choose a reason for hiding this comment

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

Punctuation: use Oxford comma here and in the other related places please

Suggested change
'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree" or "content-type"'
'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree", or "content-type"'

)->addOption(
'content-ids',
null,
InputOption::VALUE_OPTIONAL,
'Comma-separated list of content ID\'s to refresh (deleted/updated/added). Implies "no-purge", cannot be combined with "since" or "subtree"'
'Comma-separated list of content ID\'s to refresh (deleted/updated/added). Implies "no-purge", cannot be combined with "since", "subtree" or "content-type"'
)->addOption(
'subtree',
null,
InputOption::VALUE_OPTIONAL,
'Location ID whose subtree will be indexed (including the Location itself). Implies "no-purge", cannot be combined with "since" or "content-ids"'
'Location ID whose subtree will be indexed (including the Location itself). Implies "no-purge", cannot be combined with "since", "content-ids" or "content-type"'
)->addOption(
'processes',
null,
InputOption::VALUE_OPTIONAL,
'Number of child processes to run in parallel for iterations, if set to "auto" it will set to number of CPU cores -1, set to "1" or "0" to disable',
'auto'
)->addOption(
'content-type',
null,
InputOption::VALUE_REQUIRED,
'Content type identifier to refresh (deleted/updated/added). Implies "no-purge", cannot be combined with "since", "subtree" or "content-ids"'
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note, outside of the scope of this PR:
We are not actually checking that conflicting options are passed.

)->setHelp(
<<<EOT
The command <info>%command.name%</info> indexes the current configured database in the configured search engine index.
Expand Down Expand Up @@ -256,6 +261,10 @@ protected function indexIncrementally(
$count = $this->gateway->countContentInSubtree($location->pathString);
$generator = $this->gateway->getContentInSubtree($location->pathString, $iterationCount);
$purge = false;
} elseif ($contentType = $input->getOption('content-type')) {
$count = $this->gateway->countContentWithContentTypeIdentifier($contentType);
$generator = $this->gateway->getContentWithContentTypeIdentifier($contentType, $iterationCount);
$purge = false;
Comment on lines +265 to +267
Copy link
Member

Choose a reason for hiding this comment

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

Given you can't add anything to that Gateway without breaking BC promise (see previous comment), I think better solution would be to either use Repository Filtering (\eZ\Publish\API\Repository\ContentService::find) or make a dedicated method in \eZ\Publish\SPI\Persistence\Content\Handler. The latter option would be more performant and require implementing cache layer (which is good).

} else {
$count = $this->gateway->countAllContent();
$generator = $this->gateway->getAllContent($iterationCount);
Expand Down
28 changes: 28 additions & 0 deletions eZ/Publish/Core/Search/Legacy/Content/IndexerGateway.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,22 @@ public function countContentInSubtree(string $locationPath): int
return (int)$query->execute()->fetchOne();
}

public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue reported is relevant. Narrowing down of return type is available for PHP 7.4+, and this version of ezplatform-kernel supports PHP 7.3, which means you cannot use this feature.

Suggested change
public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator
public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): iterable

{
$query = $this->buildQueryForContentWithContentTypeIdentifier($contentTypeIdentifier);

yield from $this->fetchIteration($query->execute(), $iterationCount);
}

public function countContentWithContentTypeIdentifier(string $contentTypeIdentifier): int
{
$query = $this->buildCountingQuery(
$this->buildQueryForContentWithContentTypeIdentifier($contentTypeIdentifier)
);

return (int)$query->execute()->fetchOne();
}

public function getAllContent(int $iterationCount): Generator
{
$query = $this->buildQueryForAllContent();
Expand Down Expand Up @@ -101,6 +117,18 @@ private function buildQueryForContentInSubtree(string $locationPath): QueryBuild
->setParameter('path', $locationPath . '%', ParameterType::STRING);
}

private function buildQueryForContentWithContentTypeIdentifier(string $contentTypeIdentifier): QueryBuilder
{
return $this->connection->createQueryBuilder()
->select('DISTINCT c.id')
->from('ezcontentobject', 'c')
->innerJoin('c', 'ezcontentclass', 'cc', 'cc.id = c.contentclass_id')
->where('c.status = :status')
->andWhere('cc.identifier LIKE :identifier')
->setParameter('status', ContentInfo::STATUS_PUBLISHED, ParameterType::INTEGER)
->setParameter('identifier', $contentTypeIdentifier, ParameterType::STRING);
}

private function buildQueryForAllContent(): QueryBuilder
{
return $this->connection->createQueryBuilder()
Expand Down
12 changes: 12 additions & 0 deletions eZ/Publish/SPI/Search/Content/IndexerGateway.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ public function getContentInSubtree(string $locationPath, int $iterationCount):
*/
public function countContentInSubtree(string $locationPath): int;

/**
* @throws \Doctrine\DBAL\Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as the indexing is concerned, I'm not sure if we want to declare that this can throw this exception. The fact that we are using Doctrine is irrelevant for indexing (at least for the interface). @alongosz ?

Copy link
Member

Choose a reason for hiding this comment

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

As far as the indexing is concerned, I'm not sure if we want to declare that this can throw this exception. The fact that we are using Doctrine is irrelevant for indexing (at least for the interface). @alongosz ?

You're right. On SPI level we can throw SPI or API exceptions.

*
* @return \Generator list of Content IDs for each iteration
*/
public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator;

/**
* @throws \Doctrine\DBAL\Exception
*/
public function countContentWithContentTypeIdentifier(string $contentTypeIdentifier): int;

/**
* @throws \Doctrine\DBAL\Exception
*
Expand Down