This makes it easier to follow the flow of the code where sort() is called on the entity class with uasort().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Needs review
FileSize
2.71 KB
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me. Documentation-only patch.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Sounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).

Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct? It wasn't obvious to me whether or not the references to the classes were correct, just looking at the patch. So I'd just like a bit of reassurance. :)

joachim’s picture

> Sounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).

Core has 183 occurrences of that!

ack --noheading --nobreak '@see Drupal\\' | wc -l
// Says 183
ack --noheading --nobreak '@see \\Drupal\\' | wc -l
// Says 626

I'll wait for a review of the classes before rerolling for the initial slash.

jhodgdon’s picture

Yes, Core is often not following our standards, but we do have one that says if you use namespaces in docs, they *must* be fully qualified.
https://drupal.org/node/1354#classes

The previous patch was reviewed, so you might as well fix it now? :)

joachim’s picture

> Yes, Core is often not following our standards

:D

> Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct?

I mean that bit. Makes more sense to wait for any comments on that front, and reroll then?

longwave’s picture

Yes, I checked them all :) Details as follows:

ConfigEntityListController is a generic list controller for ConfigEntityBase objects, so ConfigEntityBase::sort() is correct.

BlockListController lists Block entities, which have their own overridden sort function, so Block::sort() is correct.

LanguageListController lists Language entities, which do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.

RoleStorageController handles Role entities, which again do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.

joachim’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

Updated patch.

I've removed the full stops from the end of the @see lines, as AFAICT that's the correct format.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. @see needs no trailing stop, though core does not always conform to this.

$ git grep "@see.*[^\\.]$" | wc -l
2702
$ git grep "@see.*\\.$" | wc -l
136
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Should we even use @see in inline documentation? It has a specific use in /** */ comments because those turn into links and a See Also section... ??? It's probably better to just say "See blah blah blah" in in-line docs?

longwave’s picture

Status: Needs review » Reviewed & tested by the community

We use it in enough other places already:

$ git grep "// @see" | wc -l
154

16 inline @see references to classes:

$ git grep "// @see.*Drupal.*::"
core/includes/bootstrap.inc:  // @see Drupal\simpletest\TestBase::prepareConfigDirectories()
core/lib/Drupal/Core/Config/CachedStorage.php:    // @see Drupal\Core\Cache\CacheBackendInterface::getMultiple()
core/lib/Drupal/Core/Entity/DatabaseStorageController.php:        // @see Drupal\Core\Entity\EntityInterface::__construct()
core/lib/Drupal/Core/Entity/DatabaseStorageController.php:      // @see Drupal\Core\Entity\EntityInterface::__construct()
core/lib/Drupal/Core/StreamWrapper/PublicStream.php:      // @see Drupal\simpletest\WebTestBase::setUp()
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php:        // @see \Drupal\Core\TypedData\Validation\PropertyContainerMetadata::accept();
core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php:      // @see Drupal\Core\Entity\FeedStorageController::postSave()
core/modules/ckeditor/js/ckeditor.admin.js:      // @see \Drupal\ckeditor\Plugin\editor\editor\CKEditor::settingsForm.
core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.php:    // @see Drupal\simpletest\TestBase::error()
core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverrideWebTest.php:    // @see \Drupal\Core\PathProcessor::processInbound()
core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUIRouteTest.php:    // @see \Drupal\field_ui\FieldOverview::getRegions()
core/modules/filter/filter.module:  // @see \Drupal\filter\Entity\FilterFormat::save()
core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php:      // @see Drupal\Core\Entity\EntityInterface::__construct()
core/modules/serialization/tests/serialization_test/lib/Drupal/serialization_test/SerializationTestEncoder.php:    // @see Drupal\serialization_test\SerializationTestNormalizer::normalize().
core/modules/statistics/lib/Drupal/statistics/Tests/Views/IntegrationTest.php:    // @see \Drupal\statistics\Tests\StatisticsLoggingTest::testLogging().
core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php:    // @see Drupal\simpletest\WebTestBase::xpath()
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

But "we're doing it a lot" doesn't mean "it's a good idea to do it more" or "it's our standard". For instance, we're using the word "id" all over documentation in place of "ID", but it's still incorrect every time we do it.

My question is whether using @see in inline docs is a good idea, since "See" would be just as clear and is plain English, and our documentation standards generally say we should use plain English for documentation, and @ tags in /** */ headers, right?

longwave’s picture

Isn't that out of scope for this issue? Shouldn't we just get this improvement in, and then address that (and all other uses of // @see) in a followup?

jhodgdon’s picture

Um... Let's follow our current standards in this patch, which say to write inline docs in plain English, and if you want to change the standards so that we encourage using @tags in inline docs, you can do that in another issue? :)

longwave’s picture

longwave’s picture

Title: add a @see to ConfigEntityBase::sort() where used by uasort » Add inline comments referencing ConfigEntityBase::sort() where used by uasort

I guess the old title is inaccurate :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thank you. This looks fine to me.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again- committed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.