Refactor InstallController to actions. - #5578
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
I ran out of time before I could review everything here, but see below for some initial suggestions/questions based on a partial review.
|
I changed this to draft due the checkPrerequisites method. I'll update this when #5520 is done. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Just checking if this is back on your radar, @EreMaijala, now that #5520 is done.
# Conflicts: # module/VuFind/src/VuFind/Action/AbstractAction.php # module/VuFind/src/VuFind/Action/PluginManager.php
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
ff54679 to
5d971ef
Compare
|
@demiankatz I had almost lost track of this, so thanks for pulling it back on my radar. :) |
demiankatz
added
the
architecture
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
I'm still in the process of reviewing this, but I've made some adjustments to the tests in support of my work. More as soon as time permits!
| ); | ||
| $method = $this->getMinimalPhpVersionMethod(); | ||
| // Test the method in the abstract base class by instantiating a concrete class extending it: | ||
| $action = $this->getAutowiredObject(HomeAction::class); |
There was a problem hiding this comment.
I revised this to use AutowireTrait, and it saved 20 lines of code!
| * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License | ||
| * @link https://vufind.org Main Page | ||
| */ | ||
| class InstallTest extends \VuFindTest\Integration\MinkTestCase |
There was a problem hiding this comment.
I thought some Mink test coverage might be useful. We could obviously do more, but this was a minimum viable starting point.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @EreMaijala, see below for some suggestions (some of which may be out of scope, but I thought I'd mention possible improvements while I noticed them).
The bigger issue is that FixSolrAction and FixSecurityAction seem to be missing. Did something fail to get pushed up?
| if (function_exists('posix_getpwuid') && function_exists('posix_geteuid')) { | ||
| $processUser = posix_getpwuid(posix_geteuid()); | ||
| $templateParams['runningUser'] = $processUser['name']; | ||
| } |
There was a problem hiding this comment.
We do this in at least two places, I wonder if a support method like addRunningUserToTemplateParams(& $params) would be helpful.
There was a problem hiding this comment.
Done, but in a simplified way.
| // Is the sodium extension missing? | ||
| if (!defined('SODIUM_LIBRARY_VERSION')) { | ||
| $missingExtensions[] = 'sodium'; | ||
| } |
There was a problem hiding this comment.
Feels to me like refactoring this to a getMissingExtensions method would allow us to eliminate some fragile redundancy in HomeAction::checkMethodDependencies()...
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
|
@demiankatz Oops. Not sure what happened, but apparently I completely skipped Solr and Security, also when testing. Now added and refactored according to your suggestions. |
2 participants
This includes removal of the slightly cumbersome showsql action, which is replaced by directly rendering the install/showsql template in FixDatabaseAction.