Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
- #2362433: Check if core's assumptions for loading data fields is correct or not
- #2596783: Revisiting the revision tab on entity pages
- #2597333: Research/reconsider the need for $entityTypeBlackList in MultiversionManager
- #2597337: Consider using the nextId API to generate more sequential IDs.
- #2597339: Remove entityTypeToDo
- #2597341: Resolve todo in newRevisionId()
- #2597393: Revisit multiversion_form_node_type_edit_form_alter()
- #2597414: Simplify the container definition for negotiators
- #2597422: Create a better mechanism for counting revisions
- #2597430: Implement deleted_conflicts
- #2597434: Add test for bad data in doBuildTree()
- #2597442: Use a better mechanism within updateDefaultRevision()
- #2597444: Cache RevisionTreeIndex
- #2597456: Better mechanism for ignoring the login form in QueryTrait.php
- #2597464: Do security review of getWorkspaceId()
- #2597478: Switch to BERT serialization format instead of JSON inside termToBinary()
- #2597486: Add tests for more Entity types
- #2597492: Update MultiversionWebTestBase to extend KernelTestBase instead of WebTestBase to increase performance
- #2597496: Update testSaveAndLoad
- #2597508: loadEntityByUuid in testSaveAndLoad
- #2597516: Remove unnecessary clone
- #2597526: Rename to doDelete for consistency with other storage handlers.
- #2597530: Evaluate if loadTree() is necessary in TermStorage
- #2597534: Figure out why we need to delete related comments from NodeStorage::delete
- #2597538: Create test for isDefaultRevision(FALSE) within ContentEntityStorageTrait::doSave()
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.
Comment | File | Size | Author |
---|---|---|---|
#16 | todo-links-2361453-16.patch | 15.82 KB | stevector |
#11 | todo-links-2361453-11.patch | 15.86 KB | stevector |
#10 | todo-links-2361453-10.patch | 3.43 KB | stevector |
#4 | todo-links-2361453-4.patch | 1 KB | stevector |
Comments
Comment #1
dixon_Comment #2
dixon_Comment #3
dixon_Comment #4
stevectorDick 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?
Comment #5
dixon_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.
Comment #6
dixon_@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?
Comment #7
stevectorHi dixon_,
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:
or
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
Comment #8
dixon_Reading the "inline {@link}" documentation is sounds like we should do this:
Based on the below example from: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...
Comment #9
stevectorCool, I'll go with that.
Comment #10
stevectorA bigger patch is coming. Here's what it looks like so far.
Comment #11
stevectorI think I got them all. I used
grep -R todo * | grep -v link
to make the list of comments to update.Comment #12
stevectorComment #14
dixon_Seems like a CI failure. I re-queued the patch.
Comment #15
dixon_Single blank line should be enough.
Double blank line seems unnecessary here.
The line breaks here seem a bit strange :)
Comment #16
stevectorGood catches. Thanks!
Comment #18
dixon_Thanks a lot Steve! This brings us one big step closer to the first alpha release!
Comment #20
jeqq