-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve directory tree read performance #30629
Closed
Closed
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
ae2c269
Improve directory tree read performance
Nazar65 dbbe8f5
Improve code style
Nazar65 bc26500
Code style fixes
Nazar65 f668723
Static test fixes
Nazar65 636b0ca
Use Magento framework Glob, codereview suggestion
Nazar65 ddd3d8a
Cover chnages with integration test
Nazar65 8f8d3ff
Static test fixes
Nazar65 bfbf89e
Fix MediaGallery uploader plugin to cover only media path
Nazar65 557a240
Static test fixes
Nazar65 86ed340
Fix path's issue
Nazar65 743e44c
Split second issue
Nazar65 5b61221
Skip recursice reading in excluded folders
Nazar65 a8baf00
End loop to avoid recursive reading
Nazar65 1b175de
magento/magento2#30629: Code review adjustments
sivaschenko 5d09b0f
magento/magento2#30629: Optimized tree creation and improved integrat…
sivaschenko 1b7743a
magento/magento2#30629: Added missing strict return type
sivaschenko f81f0f7
magento/magento2#30629: Removed unnecessary dependencies
sivaschenko 9d80b97
Merge branch '2.4-develop' of github.com:magento/magento2 into direct…
sivaschenko 979a5bf
magento/magento2#30629: Updated integration test to handle class name…
sivaschenko 8c96154
Merge branch '2.4-develop' into directory-tree-performance
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
110 changes: 110 additions & 0 deletions
110
app/code/Magento/MediaGalleryUi/Test/Integration/Directories/GetDirectoryTreeTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Magento\MediaGalleryUi\Test\Integration\Directories; | ||
|
||
use Magento\Framework\App\Filesystem\DirectoryList; | ||
use Magento\Framework\Filesystem; | ||
use Magento\Framework\Filesystem\Directory\WriteInterface; | ||
use Magento\MediaGalleryUi\Model\Directories\GetDirectoryTree; | ||
use Magento\TestFramework\Helper\Bootstrap; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
/** | ||
* Integration test for GetDirectoryTree | ||
*/ | ||
class GetDirectoryTreeTest extends TestCase | ||
{ | ||
private const TEST_FOLDER_NAME = 'folder-tree-fixture-folder'; | ||
private const TEST_SUB_FOLDER_NAME = 'folder-tree-fixture-subfolder'; | ||
|
||
/** | ||
* @var GetDirectoryTree | ||
*/ | ||
private $getFolderTree; | ||
|
||
/** | ||
* @inheritdoc | ||
*/ | ||
protected function setUp(): void | ||
{ | ||
$this->getFolderTree = Bootstrap::getObjectManager()->get(GetDirectoryTree::class); | ||
$this->getMediaDirectory()->create(self::TEST_FOLDER_NAME . '/' . self::TEST_SUB_FOLDER_NAME); | ||
} | ||
|
||
/** | ||
* @inheritdoc | ||
*/ | ||
protected function tearDown(): void | ||
{ | ||
$this->getMediaDirectory()->delete(self::TEST_FOLDER_NAME); | ||
} | ||
|
||
/** | ||
* Verify generated folder tree | ||
*/ | ||
public function testExecute(): void | ||
{ | ||
$nodeIsCreated = false; | ||
|
||
foreach ($this->getFolderTree->execute() as $node) { | ||
if ($node['data'] === self::TEST_FOLDER_NAME) { | ||
$nodeIsCreated = true; | ||
$this->assertEquals($this->getExpectedTreeNode(), $node); | ||
} | ||
} | ||
|
||
$this->assertTrue($nodeIsCreated, 'Test folder is not included in generated folder tree.'); | ||
} | ||
|
||
/** | ||
* Get formatted expected tree node | ||
* | ||
* @return array | ||
*/ | ||
private function getExpectedTreeNode(): array | ||
{ | ||
return [ | ||
'data' => self::TEST_FOLDER_NAME, | ||
'attr' => [ | ||
'id' => self::TEST_FOLDER_NAME, | ||
], | ||
'metadata' => [ | ||
'path' => self::TEST_FOLDER_NAME, | ||
], | ||
'path_array' => [ | ||
self::TEST_FOLDER_NAME | ||
], | ||
'children' => [ | ||
[ | ||
'data' => self::TEST_SUB_FOLDER_NAME, | ||
'attr' => [ | ||
'id' => self::TEST_FOLDER_NAME . '/' . self::TEST_SUB_FOLDER_NAME, | ||
], | ||
'metadata' => [ | ||
'path' => self::TEST_FOLDER_NAME . '/' . self::TEST_SUB_FOLDER_NAME, | ||
], | ||
'path_array' => [ | ||
self::TEST_FOLDER_NAME, | ||
self::TEST_SUB_FOLDER_NAME | ||
], | ||
'children' => [] | ||
] | ||
] | ||
]; | ||
} | ||
|
||
/** | ||
* Retrieve media directory with write access | ||
* | ||
* @return WriteInterface | ||
*/ | ||
private function getMediaDirectory(): WriteInterface | ||
{ | ||
return Bootstrap::getObjectManager()->get(Filesystem::class)->getDirectoryWrite(DirectoryList::MEDIA); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Glob()
class won't work with remote storages, all FS operations must go directly thoughtDriverInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shiftedreality so how we can read only directories with DriverInterface ? there is no any filter for that, iterator will go over all files for example if we have 1 million files and one directory that we need to get, it will takes 30 min with DriverInterface, instead of 0.1 sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is clearly missing a param in
search()
. I would suggest adding an optional pattern parameter to pass into theglob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shiftedreality ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, @shiftedreality. Unfortunately, changing the interface method declaration is a backward-incompatible change, so we should avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, but we can do it within the next release 2.5.0, once the branch is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shiftedreality that's a lot of time, the main idea to allow users such as @perfpcs use the New Media Gallery in the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nazar65 , I really wish I could use the new media gallery but even with the file changes, it still does not work properly, nor does the Adobe Stock sign in. I had to switch back to the old media gallery just to be able to get anything done.