Problem/Motivation

It is unclear to me why \Drupal\automatic_updates\ReadinessChecker\Filesystem::exists() itself exists 😜

It is just wrapper around file_exists()

From what I can tell from #3043521-48: [META] Update readiness checks for autoupdate (pre-flight check) it was added when the patch stopped using league/flysystem

file_exists() is called elsewhere in the module without a wrapper. Core also calls this many places directly.

Proposed resolution

Remove the method and just call file_exists() directly

if should remain the doc should be updated to explain why it is needed.

CommentFileSizeAuthor
#15 interdiff_14-15.txt2.27 KBheddn
#15 3157850-15.patch13.61 KBheddn
#14 3157850-14.patch13.61 KBtedbow
#14 interdiff-12-14.txt4.87 KBtedbow
#12 3157850-12.patch12.56 KBtedbow
#12 interdiff-8-12.txt9.45 KBtedbow
#8 interdiff-2-8.txt3.22 KBtedbow
#8 3157850-8.patch4.9 KBtedbow
#5 3157850-5.patch1.67 KBS_Bhandari
#2 3157850-2.patch1.67 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
3157850-2.patch1.67 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3157850-2.patch, failed testing. View results

S_Bhandari’s picture

S_Bhandari CreditAttribution: S_Bhandari at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » S_Bhandari
S_Bhandari’s picture

S_Bhandari CreditAttribution: S_Bhandari at Srijan | A Material+ Company for Drupal India Association commented
FileSize
3157850-5.patch1.67 KB

Added a patch for the same.

S_Bhandari’s picture

S_Bhandari CreditAttribution: S_Bhandari at Srijan | A Material+ Company for Drupal India Association commented
Assigned: S_Bhandari » Unassigned
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@S_Bhandari patch #5 is exactly the same as #2 correct? I am not sure why you uploaded it.

The current tests need to fixed to no rely on exists() method. It is protected method anyways so the tests shouldn't rely on it.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
3157850-8.patch4.9 KB
interdiff-2-8.txt3.22 KB

I think this good idea still remove but I makes the current tests break. The test mocks the class we are trying to test changes the return values of the protected methods. So I don't think it is actually testing the methods which I think should be down my just calling the public methods of the class with mocked argument objects if needed. Right now we are testing the internal logic of the class not the public interface.

It is tricky with this class because we need files to check but I think we can use org\bovigo\vfs\vfsStream as the core test \Drupal\Tests\Core\Extension\InfoParserUnitTest does.

Here is a start I have left the current test in place and started a new one for now. I can work more on this later

S_Bhandari’s picture

S_Bhandari CreditAttribution: S_Bhandari at Srijan | A Material+ Company for Drupal India Association commented

Hi @tedbow,

Sorry for that, mistakenly I overlooked the complete patch and just worked on it when I saw the test results failed. Yes, you are right about the logic of why the tests are getting failed. Thanks for explaining it better.

Thanks.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 3157850-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-8-12.txt9.45 KB
3157850-12.patch12.56 KB

Ok finished the test. I could figure out a way make the test use the real \Drupal\automatic_updates\ReadinessChecker\Filesystem::areSameLogicalDisk() because stat() does work with vfsStream.

Status: Needs review » Needs work

The last submitted patch, 12: 3157850-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-12-14.txt4.87 KB
3157850-14.patch13.61 KB

fixed the test

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
FileSize
3157850-15.patch13.61 KB
interdiff_14-15.txt2.27 KB

Checking if a few phpcs issues are now resolved.

  • heddn committed f7a81c5 on 8.x-1.x 
    Issue #3157850 by tedbow, heddn, S_Bhandari: Remove wrapper function for...

  • heddn committed 30da94b on 8.x-2.x 
    Issue #3157850 by tedbow, heddn, S_Bhandari: Remove wrapper function for...
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Fixed

Fixed on commit:

--- a/tests/src/Kernel/ReadinessChecker/ReadOnlyFilesystemTest.php
+++ b/tests/src/Kernel/ReadinessChecker/ReadOnlyFilesystemTest.php
@@ -206,6 +206,9 @@ class ReadOnlyFilesystemTest extends KernelTestBase {
  */
 class TestReadOnlyFileSystem extends ReadOnlyFilesystem {
 
+  /**
+   * {@inheritdoc}
+   */
   protected function areSameLogicalDisk($root, $vendor) {
     return FALSE;
   }

Status: Fixed » Closed (fixed)

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