EZP-31021: Revised exception and command messages #2878

DominikaK · 2019-11-18T13:56:20Z

Question	Answer
JIRA issue	EZP-31021
Bug/Improvement	no
New feature	no
Target version	`master`
BC breaks	no
Tests pass	yes/no
Doc needed	no

Revises exception messages and command output to:

use consistent terminology
use consistent phrasing and syntax
avoid language pitfalls
clarify some ambiguous messages

Related PRs:

TODO:
~~- [ ] Implement feature / fix a bug.~~
~~- [ ] Implement tests.~~
~~- [ ] Fix new code according to Coding Standards ($ composer fix-cs).~~

Ask for Code Review.
Run tests

eZ/Bundle/EzPublishCoreBundle/Command/CheckURLsCommand.php

gggeek · 2019-11-18T14:13:53Z

eZ/Bundle/EzPublishCoreBundle/Command/DeleteContentTranslationCommand.php

@@ -173,7 +173,7 @@ function ($versionLanguageCode) use ($languageCode) {
        if (empty($mainTranslationCandidates)) {
            throw new InvalidArgumentException(
                'language-code',
-                "The last Version of the Content {$contentName} has no other Translations beside the main one"
+                "The last version of Content item {$contentName} has no other translations beside the main one"


we do capitalize Content, even if it is a plain english word, because it is a specific "entity" in eZ. I think Version and Translation could also be kept capitalized, as they refer to specific entities as well...

gggeek · 2019-11-18T14:14:50Z

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

            )->addOption(
                'subtree',
                null,
                InputOption::VALUE_OPTIONAL,
-                'Location Id to index subtree of (incl self). Implies "no-purge", can not 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" or "content-ids"'


Are we sure we want to use ID ? Thoughout the codebase I think we use 'Id'...

gggeek · 2019-11-18T14:15:40Z

eZ/Bundle/EzPublishCoreBundle/Command/ResizeOriginalImagesCommand.php

@@ -145,7 +145,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
        if (!$fieldType || $fieldType->fieldTypeIdentifier !== 'ezimage') {
            $output->writeln(
                sprintf(
-                    "<error>FieldType of identifier '%s' of ContentType '%s' has to be 'ezimage', '%s' given.</error>",
+                    "<error>Field Type with identifier '%s' in Content Type '%s' must be 'ezimage', you provided '%s'.</error>",


I'd stick with FieldType and ContentType

eZ/Bundle/EzPublishMigrationBundle/Command/LegacyStorage/RegenerateUrlAliasesCommand.php

DominikaK · 2019-11-19T07:54:54Z

Regarding domain term spelling and capitalization, please see comment in the related PR on improving UI texts: ezsystems/ezplatform-admin-ui#1115 (comment)

gggeek · 2019-11-19T09:28:51Z

Re: domain term spelling and capitalization: I think it still makes sense to use capital letters to denote domain objects wherever we are talking about them, even if the difference in meaning between domain-object and english-noun is small. The rule seems clear and easy to understand, and conveys meaning more precisely.
Only using capitalization when the "meaning differs" seems more subjective.

Examples:
Content vs. content => the meaning is not that different in fact, but in eZ this domain object is too important to be misunderstood for something else. Imho a Content is just a "content item", shortened for convenience.
Location vs. location => again, the meaning is not that different. a Location is a "content item location"

One more case where I feel capitalization should be applied despite the "common meaning" being roughly the same as "technical meaning": Content Type group vs. Content Type Group => the former looks to me like a set of 'Content Type" domain objects, the latter like a single domain object (which it is).

As for the docs being written with an editorial user in mind and not for the developer: while I agree with the principle, let's not forget that those messages will be read as much (probably even more) by technical people than by non technical ones (developers, technical doc writers, admins, etc...). A common usecase is for a non technical person pasting the message into a mail or chat or jira ticket to communicate with a support person. So I would refine the principle as "make the messages crystal clear to the non technical person while preserving as much as possible adherence to the domain terminology and codebase and overall precision"

bootstrap.php

eZ/Bundle/EzPublishCoreBundle/Command/CheckURLsCommand.php

eZ/Bundle/EzPublishCoreBundle/Command/CleanupVersionsCommand.php

eZ/Bundle/EzPublishCoreBundle/Command/DebugConfigResolverCommand.php

eZ/Publish/Core/Persistence/Doctrine/DoctrineExpression.php

eZ/Publish/Core/Persistence/Legacy/URL/Gateway/DoctrineDatabase.php

eZ/Publish/Core/Repository/ContentService.php

eZ/Publish/Core/Search/Legacy/Content/Gateway/DoctrineDatabase.php

eZ/Publish/Core/Search/Legacy/Content/Location/Gateway/DoctrineDatabase.php

bootstrap.php

eZ/Bundle/EzPublishCoreBundle/Command/CopySubtreeCommand.php

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

eZ/Bundle/EzPublishCoreBundle/Command/UpdateTimestampsToUTCCommand.php

eZ/Bundle/EzPublishMigrationBundle/Command/LegacyStorage/RegenerateUrlAliasesCommand.php

alongosz · 2019-11-28T17:01:39Z

Passing build with linked Solr Bundle changes (ezsystems/ezplatform-solr-search-engine#162): https://travis-ci.org/ezsystems/ezpublish-kernel/builds/618278829

alongosz

Great work @DominikaK 🎉

There are a few things we need to discuss ;)

.../EzPublishCoreBundle/DependencyInjection/Compiler/FieldTypeParameterProviderRegistryPass.php

eZ/Bundle/EzPublishCoreBundle/Imagine/AliasGenerator.php

eZ/Bundle/EzPublishCoreBundle/Imagine/PlaceholderAliasGenerator.php

eZ/Bundle/EzPublishMigrationBundle/Command/LegacyStorage/RegenerateUrlAliasesCommand.php

eZ/Publish/Core/Helper/TranslationHelper.php

eZ/Publish/Core/Limitation/ContentTypeLimitationType.php

alongosz · 2019-11-28T21:05:47Z

eZ/Publish/Core/MVC/Symfony/Controller/Content/ViewController.php

@@ -152,7 +152,7 @@ public function viewLocation($locationId, $viewType, $layout = false, array $par
            } else {
                $location = $this->getRepository()->getLocationService()->loadLocation($locationId);
                if ($location->invisible) {
-                    throw new NotFoundHttpException("Location #$locationId cannot be displayed as it is flagged as invisible.");
+                    throw new NotFoundHttpException("Cannot display Location $locationId because it is flagged as invisible.");


Similar comment about removing hash as before.

eZ/Publish/Core/MVC/Symfony/Routing/UrlAliasRouter.php

eZ/Publish/Core/Persistence/Legacy/Tests/TestCase.php

Co-Authored-By: Michal Milc <[email protected]>

Co-Authored-By: Andrew Longosz <[email protected]>

DominikaK requested review from alongosz and a team November 18, 2019 13:56

gggeek reviewed Nov 18, 2019

View reviewed changes

eZ/Bundle/EzPublishCoreBundle/Command/CheckURLsCommand.php Outdated Show resolved Hide resolved

gggeek reviewed Nov 18, 2019

View reviewed changes

eZ/Bundle/EzPublishMigrationBundle/Command/LegacyStorage/RegenerateUrlAliasesCommand.php Show resolved Hide resolved

michalmilc90 suggested changes Nov 19, 2019

View reviewed changes

michalmilc90 reviewed Nov 19, 2019

View reviewed changes

bootstrap.php Outdated Show resolved Hide resolved

MagdalenaZuba suggested changes Nov 19, 2019

View reviewed changes

MagdalenaZuba approved these changes Nov 20, 2019

View reviewed changes

DominikaK changed the title ~~EZP-31021: Revise exception & command microcopy~~ EZP-31021: Revised exception and command messages Nov 20, 2019

alongosz force-pushed the ezp-31021-messages-revision branch from f2aaa10 to f794e6e Compare November 28, 2019 16:42

alongosz added Improvement Rebase required labels Nov 28, 2019

alongosz requested changes Nov 28, 2019

View reviewed changes

DominikaK requested a review from alongosz December 2, 2019 07:56

DominikaK and others added 2 commits December 2, 2019 11:10

Revised exception and command messages

9211322

Co-Authored-By: Michal Milc <[email protected]>

[Tests] Aligned tests with the changes

dcb56a3

Co-Authored-By: Andrew Longosz <[email protected]>

alongosz force-pushed the ezp-31021-messages-revision branch from 4e6f317 to dcb56a3 Compare December 2, 2019 10:10

alongosz approved these changes Dec 2, 2019

View reviewed changes

alongosz removed the Rebase required label Dec 2, 2019

michalmilc90 approved these changes Dec 2, 2019

View reviewed changes

DominikaK added the Ready for MERGE label Dec 2, 2019

alongosz merged commit 1feab64 into ezsystems:master Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-31021: Revised exception and command messages #2878

EZP-31021: Revised exception and command messages #2878

DominikaK commented Nov 18, 2019 •

edited

Loading

gggeek Nov 18, 2019

gggeek Nov 18, 2019

gggeek Nov 18, 2019 •

edited

Loading

DominikaK commented Nov 19, 2019

gggeek commented Nov 19, 2019

alongosz commented Nov 28, 2019

alongosz left a comment

alongosz Nov 28, 2019

EZP-31021: Revised exception and command messages #2878

EZP-31021: Revised exception and command messages #2878

Conversation

DominikaK commented Nov 18, 2019 • edited Loading

gggeek Nov 18, 2019

Choose a reason for hiding this comment

gggeek Nov 18, 2019

Choose a reason for hiding this comment

gggeek Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

DominikaK commented Nov 19, 2019

gggeek commented Nov 19, 2019

alongosz commented Nov 28, 2019

alongosz left a comment

Choose a reason for hiding this comment

alongosz Nov 28, 2019

Choose a reason for hiding this comment

DominikaK commented Nov 18, 2019 •

edited

Loading

gggeek Nov 18, 2019 •

edited

Loading