User "Tim Starling" changed the status of MediaWiki.r88929. Old Status: new New Status: fixme
User "Tim Starling" also posted a comment on MediaWiki.r88929. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/88929#c17848 Commit summary: Commiting here, because other problems exist on trunk. This needs to be forward-ported to trunk. * Make Pg installation work for lesser privileged role in Postgres (i.e. not super user, but can create users and databases) for Bug #28845. * Switch to Pg's new “role” tables to replace the old “user” nes * Give DatabaseInstaller::openConnection() the ability to select a db since there isn't any working selectDB method for Pg yet. * If the installing role is the same as the one that the wiki will use, make sure it can see the tables in the MW schema * Remove addition of user_hidden field to the mwtable in Pg installation since it isn't referred to anywhere and breaks the installation. * Remove the word “below” from the config-connection-error message since sometimes the message is displayed where there is no login information shown at the same time. Comment: There are some style problems here: single line if statements, comment style, attempted vertical alignment in DatabaseInstaller::getConnection(). <pre> + if( $db === null ) throw new DBConnectionError("Unknown problem while connecting."); </pre> How could this possibly happen? You just unconditionally assigned $db to an object. <pre> - $rights = $conn->selectField( 'pg_catalog.pg_user', - 'CASE WHEN usesuper IS TRUE THEN - CASE WHEN usecreatedb IS TRUE THEN 3 ELSE 1 END - ELSE CASE WHEN usecreatedb IS TRUE THEN 2 ELSE 0 END - END AS rights', - array( 'usename' => $superuser ), __METHOD__ + $rights = $conn->selectField( 'pg_catalog.pg_roles', + 'CASE WHEN rolsuper then 1 + WHEN rolcreatedb then 2 + ELSE 3 + END as rights', + array( 'rolname' => $superuser ), __METHOD__ ); </pre> By criticising this query on bug 28845 I was trying to hint that it would be better to get rid of the CASE and the integer literals altogether, and to put the logic in PHP instead. <pre> - public function getConnection() { + public function getConnection( $dbName = null ) { </pre> You broke the caching in getConnection(). Now if someone calls getConnection() with $dbName non-null, it will pollute $this->db and cause subsequent calls with $dbName=null to return a connection to the wrong database. It was already broken by r81440 (which was backported), but now it's broken even more. See the way this was dealt with in Oracle in r81084 and r83017. Also there is the problem that the $dbName parameter is undocumented and is ignored for most of the DBMSes. <pre> - $this->db->query("ALTER USER $safeuser SET search_path = $safeschema"); + $this->db->query("ALTER ROLE $safeuser LOGIN"); } } + $this->db->query("ALTER ROLE $safeuser SET search_path = $safeschema, public"); </pre> By making the "ALTER ROLE SET search_path" unconditional, instead of only being done for new users, this will have the effect of destroying any existing account, breaking any wiki that uses it other than the one that is being installed. It's probably better to let the new wiki be broken rather than break unrelated wikis and apps. Is there a bug report for the issue that this is trying to fix? _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
