Problem/Motivation

There are numerous todo comments in the code without a clear plan for resolving each one. A sensible next step is to ensure that each todo at least has a related drupal.org issue number associated with it.

Proposed resolution

Update all todo comments with links to relevant issues. Create the issues if necessary.

The following issues were created or referenced in order to complete this issue.

Remaining tasks

Review this patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Original report by [username]

As a part of this ticket all the @todo's should also be removed from the source code.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dixon_’s picture

Title: [meta] Create issues for all @todo's in the code » Create issues for all @todo's in the code
dixon_’s picture

Issue summary: View changes
dixon_’s picture

Assigned: Unassigned » dixon_
stevector’s picture

Dick when you write "As a part of this ticket all the @todo's should also be removed from the source code." do you simply mean that this ticket should stay open until all todos are resolved? I imagine the first step is a patch like this one, but with a lot more additions, yes?

dixon_’s picture

do you simply mean that this ticket should stay open until all todos are resolved?

No, this issue can be closed once we've created separate issues here on d.o for all @todo's and removed those comments from the code. Your patch is right, just needs more of the same thing :)

I think it's also a good idea to drop-in all issues we create as part of this into a comment here, so it's easy to cross reference that all @todo comments got issues created for them.

dixon_’s picture

@stevector Thinking more about this, should we instead perhaps place each // @todo Foo bar with @todo See http://drupal.org/node/xxx instead? I guess that would be clearer for contributors also looking at the code?

Then each referenced issue would be responsible for removing the @todo See http://drupal.org/node/xxx part...

Thoughts?

stevector’s picture

Issue summary: View changes

Hi dixon_,

I think it's also a good idea to drop-in all issues we create as part of this into a comment here, so it's easy to cross reference that all @todo comments got issues created for them.

Sure, so far all I had done is used the "related issues" functionality when creating #2596783: Revisiting the revision tab on entity pages. I'll also compile those in the issue summary.

So, just to be really clear I think you're asking if the comments should be:

// @todo: Provide more granular permissions here.
// @see https://www.drupal.org/node/2596783.

or

// @todo: see https://www.drupal.org/node/2596783.
// Provide more granular permissions here.

I'd prefer the second because it is easier to grep and see if if this task is completed :) And for hyper-pedantry, should we actually be using "@link"? http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...

Perhaps

// @todo: @link https://www.drupal.org/node/2596783.
// Provide more granular permissions here.
dixon_’s picture

Reading the "inline {@link}" documentation is sounds like we should do this:

// @todo: {@link https://www.drupal.org/node/2596783 Provide more granular permissions}

Based on the below example from: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...

  /**
     * Text with an {@link http://www.example.com Inline Link to a Hyperlink} and an inline
     * link to {@link element()} displays without a break in the flow
     * (again, the parentheses in "element()" are only necessary
     * because it is a function)
     */
    function element()
    {
    }
stevector’s picture

Assigned: dixon_ » stevector

Cool, I'll go with that.

stevector’s picture

FileSize
3.43 KB

A bigger patch is coming. Here's what it looks like so far.

stevector’s picture

I think I got them all. I used grep -R todo * | grep -v link to make the list of comments to update.

stevector’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 11: todo-links-2361453-11.patch, failed testing.

dixon_’s picture

Seems like a CI failure. I re-queued the patch.

dixon_’s picture

  1. +++ b/multiversion_ui/src/Routing/RouteSubscriber.php
    @@ -63,7 +63,10 @@ class RouteSubscriber extends RouteSubscriberBase {
    +            // @todo: {@link https://www.drupal.org/node/2596783 Provide more
    +            // granular permissions.}
    +
    +
    

    Single blank line should be enough.

  2. +++ b/src/MultiversionManager.php
    @@ -76,7 +78,11 @@ class MultiversionManager implements MultiversionManagerInterface {
    +   * @todo: {@link https://www.drupal.org/node/2597337 Consider using the
    +   * nextId API to generate more sequential IDs.}
    +   *
    +   *
    +   * @link
    

    Double blank line seems unnecessary here.

  3. +++ b/src/Workspace/SessionWorkspaceNegotiator.php
    @@ -26,7 +26,9 @@ class SessionWorkspaceNegotiator extends WorkspaceNegotiatorBase implements Work
    -      // @todo Review from a security perspective.
    +      // @todo: {@link https://www.drupal.org/node/2597464 Review from a
    +      // security
    +      // perspective.}
           $workspace_id = $_SESSION['workspace'];
    

    The line breaks here seem a bit strange :)

stevector’s picture

  • dixon_ committed 8925264 on 8.x-1.x authored by stevector
    Issue #2361453 by stevector: Create issues for all @todo's in the code
    
dixon_’s picture

Status: Needs review » Fixed

Thanks a lot Steve! This brings us one big step closer to the first alpha release!

Status: Fixed » Closed (fixed)

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

jeqq’s picture