Fix database autocreate at install #2635

Alkarex · 2019-11-05T21:02:22Z

Several bugs prevented the auto-creation of the database in Web and CLI installs.
Fix YunoHost-Apps/freshrss_ynh#84 (comment)

Several bugs prevented the auto-creation of the database in Web and CLI installs. Fix YunoHost-Apps/freshrss_ynh#84 (comment)

Alkarex · 2019-11-05T21:25:41Z

Tested by @plopoyop

marienfressinaud · 2019-11-06T12:49:50Z

lib/lib_install.php

@@ -85,14 +85,21 @@ function checkDb() {
 		$db['pdo_options'] = [];
 	}
 	$db['pdo_options'][PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
-	$dbBase = isset($db['base']) ? $db['base'] : '';
+	$conf->db = $db;	//TODO: Remove this Minz limitation "Indirect modification of overloaded property"


I'm not sure why you need to re-set the $db in conf, but I would avoid this side effect. But maybe we can't do differently due to the actual architecture?

Exactly: FreshRSS_Context::$system_conf->db['key'] = 'value'; does not work due to the fact that Minz introduces a barrier with __get().
The error message is in the comment: Indirect modification of overloaded property

What I meant is, why checkDb changes the system conf? For me, the method should not touch the conf unless to read it.
… and writing this, I realize that maybe it's just that checkDb is badly named, perhaps something like initDb would be more self-explanatory?

Ok, forget about it ^^'

Concerning the "todo", I would add it in the lib/Minz/Configuration.php, where it has more sense (otherwise, we would add it each time we do this). I also cherish the idea of rewriting the configuration system for something simpler… one day :)

Indeed for changing the name. And yes for changing the configuration, as we are not really using its advantages (setters and getters) but we pay its drawbacks (layers that make the syntax more complex and error prone).
The comment was more to explain the strange syntax

Name changed 41179be

marienfressinaud

To complete my comment, I would appreciate a more detailed commit message to explain the changes (at least what bugs are you patching?) I'm pretty ok with the PR, but in the same time I don't feel like my review is really useful since I'm just guessing the problem(s) :(

Alkarex · 2019-11-06T13:01:21Z

app/Models/FeedDAO.php

@@ -102,7 +102,9 @@ public function addFeedObject($feed) {
 				'httpAuth' => $feed->httpAuth(),
 				'attributes' => $feed->attributes(),
 			);
-			if ($feed->mute() || $feed->ttl() != FreshRSS_Context::$user_conf->ttl_default) {


FreshRSS_Context::$user_conf is not available when creating a new user

Alkarex · 2019-11-06T13:02:36Z

app/install.php

@@ -132,7 +132,7 @@ function saveStep2() {
 		$config_array = [
 			'salt' => generateSalt(),
 			'base_url' => $base_url,
-			'default_user' => 'admin',
+			'default_user' => '_',


So that we can touch e.g. an SQLite database in the ./data/users/_/ folder, which is the only one we have so early in the install process

Alkarex · 2019-11-06T13:03:09Z

app/install.php

@@ -154,12 +154,18 @@ function saveStep2() {
 		@unlink(DATA_PATH . '/config.php');	//To avoid access-rights problems
 		file_put_contents(DATA_PATH . '/config.php', "<?php\n return " . var_export($config_array, true) . ";\n");

+		if (function_exists('opcache_reset')) {
+			opcache_reset();


We should always clear opcache before reading a dynamically generated PHP file.

Alkarex · 2019-11-06T13:03:59Z

app/install.php

 		Minz_Configuration::register('system', DATA_PATH . '/config.php', FRESHRSS_PATH . '/config.default.php');
 		FreshRSS_Context::$system_conf = Minz_Configuration::get('system');

 		$ok = false;
 		try {
+			Minz_Session::_param('currentUser', $config_array['default_user']);


This is before we actually have the name of a real user, so use default one

I'm a bit upset with this one: we need the session variable really deep in the code and it's absolutely not intuitive. I know it's not the fault of this PR at all and we'll not change it quite soon… but I'd love to keep track somehow of this kind of technical debt.

Yes, maybe by using only the Context object, and if so it would not be a big change

Alkarex · 2019-11-06T13:04:21Z

cli/do-install.php

+}
+
+Minz_Configuration::register('system', DATA_PATH . '/config.php', FRESHRSS_PATH . '/config.default.php');
+FreshRSS_Context::$system_conf = Minz_Configuration::get('system');


The configuration was not loaded

Alkarex · 2019-11-06T13:05:22Z

lib/Minz/ModelPdo.php

@@ -28,6 +28,9 @@ public function __construct($currentUser = null, $currentPdo = null) {
 		if ($currentUser === null) {
 			$currentUser = Minz_Session::param('currentUser');
 		}
+		if ($currentUser == '') {
+			throw new Minz_PDOConnectionException('Current user must not be empty!', '', Minz_Exception::ERROR);


Another barrier to prevent logic from running without a proper user. The other bugs fixed in the PR allowed coming here with a blank user

This breaks cli (cf. #2644 ) Removing this two lines is enough to make cli works again.

@X-dark Thanks for the heads up

Fixed by #2646

Alkarex · 2019-11-06T13:05:44Z

lib/lib_install.php

-	Minz_ModelPdo::$usesSharedPdo = false;
-	$databaseDAO = FreshRSS_Factory::createDatabaseDAO();
-	$databaseDAO->create();
+	if ($db['type'] !== 'sqlite') {


There is no global database to create for SQLite

Alkarex · 2019-11-06T13:06:35Z

@marienfressinaud I have added some comments

marienfressinaud

Thanks for the comments! Even if I made some comments, it's generic and out of the scope.

marienfressinaud · 2019-11-06T13:49:53Z

app/install.php

 		Minz_Configuration::register('system', DATA_PATH . '/config.php', FRESHRSS_PATH . '/config.default.php');
 		FreshRSS_Context::$system_conf = Minz_Configuration::get('system');

 		$ok = false;
 		try {
+			Minz_Session::_param('currentUser', $config_array['default_user']);


I'm a bit upset with this one: we need the session variable really deep in the code and it's absolutely not intuitive. I know it's not the fault of this PR at all and we'll not change it quite soon… but I'd love to keep track somehow of this kind of technical debt.

marienfressinaud · 2019-11-06T13:58:17Z

lib/lib_install.php

@@ -85,14 +85,21 @@ function checkDb() {
 		$db['pdo_options'] = [];
 	}
 	$db['pdo_options'][PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
-	$dbBase = isset($db['base']) ? $db['base'] : '';
+	$conf->db = $db;	//TODO: Remove this Minz limitation "Indirect modification of overloaded property"


What I meant is, why checkDb changes the system conf? For me, the method should not touch the conf unless to read it.
… and writing this, I realize that maybe it's just that checkDb is badly named, perhaps something like initDb would be more self-explanatory?

Ok, forget about it ^^'

Concerning the "todo", I would add it in the lib/Minz/Configuration.php, where it has more sense (otherwise, we would add it each time we do this). I also cherish the idea of rewriting the configuration system for something simpler… one day :)

FreshRSS#2635 (comment)

One forgotten condition. Related to FreshRSS#2646 and FreshRSS#2635

One forgotten condition. Related to #2646 and #2635

* Fix database autocreate at install Several bugs prevented the auto-creation of the database in Web and CLI installs. Fix YunoHost-Apps/freshrss_ynh#84 (comment) * initDb FreshRSS#2635 (comment)

One forgotten condition. Related to FreshRSS#2646 and FreshRSS#2635

Fix database autocreate at install

a3ed768

Several bugs prevented the auto-creation of the database in Web and CLI installs. Fix YunoHost-Apps/freshrss_ynh#84 (comment)

Alkarex added this to the 1.15.1 milestone Nov 5, 2019

Alkarex added Critical ☠️ Important 🔥 and removed Critical ☠️ labels Nov 5, 2019

Alkarex mentioned this pull request Nov 5, 2019

FreshRSS 1.15.0 YunoHost-Apps/freshrss_ynh#84

Closed

marienfressinaud reviewed Nov 6, 2019

View reviewed changes

Alkarex commented Nov 6, 2019

View reviewed changes

marienfressinaud approved these changes Nov 6, 2019

View reviewed changes

initDb

41179be

FreshRSS#2635 (comment)

Alkarex merged commit 2203015 into FreshRSS:dev Nov 6, 2019

Alkarex deleted the fix_install branch November 6, 2019 14:16

Alkarex mentioned this pull request Nov 6, 2019

FreshRSS 1.15.1 #2633

Merged

X-dark mentioned this pull request Nov 8, 2019

cli failing with Current user must not be empty #2644

Closed

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Nov 8, 2019

Fix CLI install for SQLite

a4b2cc3

One forgotten condition. Related to FreshRSS#2646 and FreshRSS#2635

Alkarex mentioned this pull request Nov 8, 2019

Fix CLI install for SQLite #2648

Merged

Alkarex added a commit that referenced this pull request Nov 9, 2019

Fix CLI install for SQLite (#2648)

6a643d1

One forgotten condition. Related to #2646 and #2635

javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020

Fix CLI install for SQLite (FreshRSS#2648)

1093569

One forgotten condition. Related to FreshRSS#2646 and FreshRSS#2635

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix database autocreate at install #2635

Fix database autocreate at install #2635

Alkarex commented Nov 5, 2019 •

edited

Loading

Alkarex commented Nov 5, 2019

marienfressinaud Nov 6, 2019

Alkarex Nov 6, 2019

marienfressinaud Nov 6, 2019

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

marienfressinaud left a comment

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

marienfressinaud Nov 6, 2019

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

Alkarex Nov 6, 2019

X-dark Nov 8, 2019

Alkarex Nov 8, 2019

Alkarex Nov 8, 2019

Alkarex Nov 6, 2019

Alkarex commented Nov 6, 2019

marienfressinaud left a comment

marienfressinaud Nov 6, 2019

marienfressinaud Nov 6, 2019

Fix database autocreate at install #2635

Fix database autocreate at install #2635

Conversation

Alkarex commented Nov 5, 2019 • edited Loading

Alkarex commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marienfressinaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alkarex commented Nov 6, 2019

marienfressinaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alkarex commented Nov 5, 2019 •

edited

Loading