Refactor NDB_Page::getCSS/JSDependencies()
#9074
Open
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.
Brief summary of changes
While browsing the Loris code, I noticed that the methods
NDB_Page::getCSSDependencies()
andNDB_Page::getJSDependencies()
are defined in a way that forces each redefinition to duplicate (more than 50 times) the appending of the dependencies and the prepending of the Loris base URL to each dependency. This is bad for several reasons : duplication (duplicated code), complexity (repeated use ofparent
/array_merge
), verbosity (because of the duplicated code), fragility (forgetting to duplicate the logic leads to a bug).I propose this simplified model that divides
getCSS/JSDependencies()
method in two:NDB_Page::getCSS/JSDependencies()
, redefined in each final class that returns the page-specific dependencies of that class.NDB_Page::getAllCSS/JSDependencies()
, defined only inNDB_Page
, that calls the previous method, appends the returned page-specific dependencies to the global ones, and prepends the Loris base URL to each dependency.If I take a random file, here is the result:
Before
After
Further thoughts
$baseURL
, thus enabling its factoring. If there is ever a need for an external dependency, it is possible to creategetExternalDependencies()
methods. I think segregating internal and external dependencies is a good thing.NDB_Menu_Filter
, which however only includes library React components (tables...) that should already be imported by each module that uses them.@inheritDoc
, which the linter does not seem to detect right now. I'll also add that many previous comments were outdated (listing files that were no longer there) or inconsistent with one another (different formats for the same meaning).Testing instructions
Links to related issues