Modify comment referring to file_directory_temp()

Added a patch for the same.

I think we should replace the function file_directory_temp() (as this is deprecated) with \Drupal::service('file_system')->getTempDirectory();. So, instead of removing the comment fully we should just replace the deprecated function with the code.

Thanks.

Hi @nitvirus,

Thanks for reviewing the issue. The original requirement here was to remove the comment referring to file_directory_temp() and that has been done in this patch. Also, I think the point that you are depicting here has already been addressed. Please do have a check and let me know if there are any other thoughts on this.

Thanks.

Sure,

Correct @S_Bhandari, the scope defines we should remove the comment.
The patch looks fine , can we also remove the empty line coming on top of comments.

Sure, I will work on this.

Thanks.

I think this still could be a novice issue because it needs an issue summary update to figure out why the comment exists.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php
@@ -74,8 +74,6 @@ protected function connect() {
         $connection_info['default']['database'] = \Drupal::service('file_system')->tempnam(sys_get_temp_dir(), 'sqlite');

I think the comment may have some important history. Can you update the issue summary after researching why/when the comment got into the code base in the first place? This could be going into drupal 7's old directory structure for database drivers.

Hi @nitvirus,

Added a patch for the observation you have shared. Please review the same and let me know for any other observation on this.

Thanks.

Hi @mradcliffe,

I did research on when the comment got into the code base in the first place by running the git blame on the file and I can see that it is added in the commit ID - a5a2ce92, on 2012-11-20 10:57:13. I also have looked into the commit ID by running git show [COMMIT_ID] and can confirm that it was added in this commit only. Please do let me know if this information is fine with you.

Thanks.

That's a good start. Finding the commit allows us to find that it was added in #203955: Create database at installation time. A really old issue. Back then, issues sometimes got multiple commits, spanned multiple versions. The patch that ended up being committed, that's relevant is https://www.drupal.org/project/drupal/issues/203955#comment-6698374.

We need to dig into that issue to starting from that comment, going up, and see if there was any discussion or reason why it was added. It wasn't in the initial several patches in the issue that I found.

We're looking for the explanation to determine if that comment should be removed in this patch or not.

The comment appears for the first time in the patch file in comment #36.

file_directory_temp() is deprecated and moved to the FileSystem service. See change notice.

\Drupal::service('file_system')->getTempDirectory() also requires database access.

Tested by setting a faulty database connection and then executing a
drush php-eval "var_dump(\Drupal::service('file_system')->getTempDirectory());"
command.

The attached patch changes the comment to:

// We cannot use \Drupal::service('file_system')->getTempDirectory()
// here because we haven't yet successfully connected to the database.

Issue summary updated accordingly.

The patch file on #14 is working. Tested on 9.2 branch.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!