Bug 1873965 - Revert changes that made the attach_inlines parameter to differential.createcomment a no-op

[suhaibmujahid]

To restore the functionality of attach_inlines, I implemented the following changes:

• Reverts commit 3976488
• Copy the loadUnsubmittedInlineComments function from

  phabricator/src/applications/differential/query/DifferentialTransactionQuery.php

  Lines 10 to 53 in 3976488

  	public static function loadUnsubmittedInlineComments(
  	PhabricatorUser $viewer,
  	DifferentialRevision $revision) {
  	$inlines = id(new DifferentialDiffInlineCommentQuery())
  	->setViewer($viewer)
  	->withRevisionPHIDs(array($revision->getPHID()))
  	->withAuthorPHIDs(array($viewer->getPHID()))
  	->withHasTransaction(false)
  	->withIsDeleted(false)
  	->needReplyToComments(true)
  	->execute();
  	foreach ($inlines as $key => $inline) {
  	$inlines[$key] = DifferentialInlineComment::newFromModernComment(
  	$inline);
  	}
  	PhabricatorInlineComment::loadAndAttachVersionedDrafts(
  	$viewer,
  	$inlines);
  	// Don't count void inlines when considering draft state.
  	foreach ($inlines as $key => $inline) {
  	if ($inline->isVoidComment($viewer)) {
  	unset($inlines[$key]);
  	continue;
  	}
  	// For other inlines: if they have a nonempty draft state, set their
  	// content to the draft state content. We want to submit the comment
  	// as it is currently shown to the user, not as it was stored the last
  	// time they clicked "Save".
  	$draft_content = $inline->getContentForEdit($viewer);
  	if (strlen($draft_content)) {
  	$inline->setContent($draft_content);
  	}
  	}
  	$inlines = mpull($inlines, 'getStorageObject');
  	return $inlines;
  	}

• Comment out the section of loadUnsubmittedInlineComments that uses the getContentForEdit method because it was removed

[suhaibmujahid]

Testing on Phabricator-Dev:

Call to undefined method DifferentialTransactionQuery::loadUnsubmittedInlineComments()

Depth	Library	File	Where
7	phabricator	[applications/conduit/method/ConduitAPIMethod.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/method/ConduitAPIMethod.php$156) : 156	DifferentialCreateCommentConduitAPIMethod::execute()
6	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$131) : 131	ConduitAPIMethod::executeMethod()
5	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$81) : 81	ConduitCall::executeMethod()
4	phabricator	[applications/conduit/controller/PhabricatorConduitAPIController.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/controller/PhabricatorConduitAPIController.php$83) : 83	ConduitCall::execute()
3	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$285) : 285	PhabricatorConduitAPIController::handleRequest()
2	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$205) : 205	AphrontApplicationConfiguration::processRequest()
1		        /app/phabricator/webroot/index.php : 35	AphrontApplicationConfiguration::runHTTPRequest()

[suhaibmujahid]

{"result":null,"error_code":"ERR-CONDUIT-CORE","error_info":"Querying for comments that are \"not published\" is not supported."}

[suhaibmujahid]

Argument 1 passed to PhabricatorApplicationTransaction::attachComment() must be an instance of PhabricatorApplicationTransactionComment, instance of DifferentialInlineComment given, called in /app/phabricator/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php on line 121

Depth	Library	File	Where
8	phabricator	[applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php$121) : 121	PhabricatorApplicationTransaction::attachComment()
7	phabricator	[applications/conduit/method/ConduitAPIMethod.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/method/ConduitAPIMethod.php$156) : 156	DifferentialCreateCommentConduitAPIMethod::execute()
6	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$131) : 131	ConduitAPIMethod::executeMethod()
5	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$81) : 81	ConduitCall::executeMethod()
4	phabricator	[applications/conduit/controller/PhabricatorConduitAPIController.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/controller/PhabricatorConduitAPIController.php$83) : 83	ConduitCall::execute()
3	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$285) : 285	PhabricatorConduitAPIController::handleRequest()
2	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$205) : 205	AphrontApplicationConfiguration::processRequest()
1		        /app/phabricator/webroot/index.php : 35	AphrontApplicationConfiguration::runHTTPRequest()

[suhaibmujahid]

Call to undefined method DifferentialInlineComment::getContentForEdit()

Depth	Library	File	Where
8	phabricator	[applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php$100) : 100	DifferentialCreateCommentConduitAPIMethod::loadUnsubmittedInlineComments()
7	phabricator	[applications/conduit/method/ConduitAPIMethod.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/method/ConduitAPIMethod.php$156) : 156	DifferentialCreateCommentConduitAPIMethod::execute()
6	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$131) : 131	ConduitAPIMethod::executeMethod()
5	phabricator	[applications/conduit/call/ConduitCall.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/call/ConduitCall.php$81) : 81	ConduitCall::executeMethod()
4	phabricator	[applications/conduit/controller/PhabricatorConduitAPIController.php](https://secure.phabricator.com/diffusion/P/browse/master/src/applications/conduit/controller/PhabricatorConduitAPIController.php$83) : 83	ConduitCall::execute()
3	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$285) : 285	PhabricatorConduitAPIController::handleRequest()
2	phabricator	[aphront/configuration/AphrontApplicationConfiguration.php](https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/configuration/AphrontApplicationConfiguration.php$205) : 205	AphrontApplicationConfiguration::processRequest()
1		        /app/phabricator/webroot/index.php : 35	AphrontApplicationConfiguration::runHTTPRequest()

[suhaibmujahid]

Finally, it is working on phabricator-dev!

[zzzeid]

I think it makes sense to include a more comprehensive commit message, perhaps referencing the different commits that had to be reverted (looks like more than one? I.e., loadUnsubmittedInlineComments and loadAndAttachVersionedDrafts). Also including the a quick overview of functionality that was added/changed and why in the commit message.

[suhaibmujahid]

I think it makes sense to include a more comprehensive commit message, perhaps referencing the different commits that had to be reverted (looks like more than one? I.e., loadUnsubmittedInlineComments and loadAndAttachVersionedDrafts). Also including the a quick overview of functionality that was added/changed and why in the commit message.

I updated the pull request's title and description, which can be reflected on the commit message when you "Squash and merge" the PR.

[zzzeid]

This looks good to me. One minor nit, and the only other thing which I mentioned on Slack was whether we want to consider reintroducing this under a different parameter, to avoid unexpected behaviour for anyone using this parameter already (which may not be an issue anyway).

[suhaibmujahid]

This looks good to me. One minor nit, and the only other thing which I mentioned on Slack was whether we want to consider reintroducing this under a different parameter, to avoid unexpected behaviour for anyone using this parameter already (which may not be an issue anyway).

I don't think it's necessary because it won't have an impact unless the same user has previously created inline comment drafts on the same revision.