Issue #3226570: Ensure exclusions are working
Merge request reports
Activity
added 85 commits
-
36e0ad7a...2476a5d8 - 82 commits from branch
project:8.x-2.x - 1f4ec448 - Merge branch '8.x-2.x' into test-exclusions-kernel
- f6ca4c0f - Improve the test
- 47c4f7ab - Add kernel test
Toggle commit list-
36e0ad7a...2476a5d8 - 82 commits from branch
113 124 $event->excludePath($this->sitePath . DIRECTORY_SEPARATOR . $settings_file); 114 125 $event->excludePath($default_site . DIRECTORY_SEPARATOR . $settings_file); 115 126 } 127 128 // If the database is SQLite, it might be located in the active directory 129 // and we should not stage it. 130 if ($this->database->driver() === 'sqlite') { 131 $options = $this->database->getConnectionOptions(); 132 $database = str_replace($this->appRoot, NULL, $options['database']); 133 $database = ltrim($database, '/'); 134 $event->excludePath($database); 135 $event->excludePath("$database-shm"); 136 $event->excludePath("$database-wal"); 
AFAICT, there is no benefit to trying to be "smart" about the paths. If there's redundancy in the exclusions list (i.e., the database is in the files directory), it's harmless. Those things will just be excluded as per usual, with no ill effect that I'm aware of. Trying to make it clever merely increases the possibility of bugs.
Edited by Adam G-H
73 $connection = $this->prophesize(Connection::class); 74 $connection->driver()->willReturn('sqlite'); 75 $connection->getConnectionOptions()->willReturn(['database' => $database]); 76 77 $subscriber = new ExcludedPathsSubscriber( 78 $this->getDrupalRoot(), 79 'sites/default', 80 $this->container->get('file_system'), 81 $this->container->get('stream_wrapper_manager'), 82 $connection->reveal() 83 ); 84 85 $event = new PreCreateEvent($this->createStage()); 86 $subscriber->preCreate($event); 87 // All of the expected exclusions should be flagged. 88 $this->assertEmpty(array_diff($expected_exclusions, $event->getExcludedPaths())); - Comment on lines +87 to +88
I think if someone looked at this test class after this issue it would not be clear at all why this test method just assert the exclusions are exactly equal to expected array of exclusions, basically we do we ignore the file system exclusions.
I think we should either cover this or make a follow-up to. I am also wondering about the .git folder special logic now. I think if you do a composer require and
prefer-sourcewon't there be a .git folder? -
it would not be clear at all why this test method just assert the exclusions are exactly equal to expected array of exclusions
Huh? That's not what this is asserting. We're passing in a certain array of exclusions, and we want to make sure those are included in the list of exclusions collected by the event, not that the expected exclusions are the only paths collected by the event. Maybe a comment and a few renames would clarify this?
-
Sorry I mistyped my comment above. This what mean to say
I think if someone looked at this test class after this issue it would not be clear at all why this test method doesn't just assert the exclusions are exactly equal to an expected array of exclusions, basically why do we ignore the file system exclusions in our test and simple test logic.
So I think we should either change this from
testSqliteDatabaseExcluded()totestExclusions()to cover all the logic in the this 1 class or make a follow-up to add this coverage.
added 1 commit
- ca705cdc - Remove test from old iteration of this patch