nuvoleweb / ui_patterns

[NOTE] Development has moved to https://drupal.org/project/ui_patterns
https://drupal.org/project/ui_patterns
GNU General Public License v2.0
85 stars 56 forks source link

NotRegularDirectoryException thrown while scanning for Yaml Patterns Deriver #293

Closed gambry closed 4 years ago

gambry commented 4 years ago

The work #291 wrongly updated the deprecated file_scan_directory($directory) to simply Drupal::service('file_system')->scanDirectory($directory). According to the change record the code should have been:

if (is_dir($directory)) {
  $files = \Drupal::service('file_system')->scanDirectory($directory);
}

Because of that AbstractYamlPatternsDeriver::fileScanDirectory() expects for every module and theme to have a templates/ directory which if missing on any will cause a fatal error with NotRegularDirectoryException and so making the websites unusable.

yenyasinn commented 4 years ago

expects for every module and theme to have a templates/ directory. It is incorrect. AbstractYamlPatternsDeriver::fileScanDirectory() scans all folders in themes and modules provided by LibraryDeriver::getDirectories(). It doesn't matter is there template folder or not.

The work #291 wrongly updated the deprecated file_scan_directory($directory). If by some occasion module directory isn't a directory (hmm, how can it be? Is module removed but cache isn't cleared? ) then NotRegularDirectoryException is thrown by FileSystem::scanDirectory().

Solution is provided https://github.com/nuvoleweb/ui_patterns/pull/301

gambry commented 4 years ago

@yenyasinn I don't get your comment. My point was the change was done incorrectly, as previously file_scan_dirctory() was checking internally for the existence of the directory and if it exists then proceeding with the scan. The changed code wrongly assumed the existence was still covered by the new service, while it wasn't.

Thanks for the fix.

backlineint commented 4 years ago

A similar issue was reported against UI Patterns Pattern Lab, which I believe would be resolved by this fix.

ademarco commented 4 years ago

PR merged, thank you everyone!