Ref T9897. This moves "Domain" to "DomainFullURI" to allow setting of https or for some reason, a port. I guess.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T9897: Unbeta Feedback on Phame
- Commits
- rPaf5001db642d: Allow PhameBlog to take a full URI instead of just a domain name
Try to break by setting a path, or fake protocol. Set to http, or https, see correct redirects. Verify domain still gets written.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I think I caught a couple of issues, let me know if they actually work?
Since we're completely replacing the TYPE_DOMAIN transaction, can we just nuke all of it and then optionally do this to convert older transaction stories?
UPDATE {$NAMESPACE}_phame.phame_blogtransaction
SET transactionType = 'phame.blog.full.domain' WHERE transactionType = 'phame.blog.domain';| src/applications/phame/editor/PhameBlogEditor.php | ||
|---|---|---|
| 85–88 | I expect this may not work if you set domains on two blogs, then remove the domains on both of them, since it will try to set both domains to '' (empty string) and cause a unique key violation. If that does create an issue, something like this may fix it: $new_value = $xaction->getNewValue();
if (strlen($new_value)) {
// do logic as written, to extract and set domain
} else {
$object->setDomain(null);
}
$object->setDomainFullURI($new_value);
return; | |
| 199–210 | This probably doesn't work anymore -- I think it's checking the full domain ("http://domain.com/") against the bare domain ("domain.com"). Specifically, I'd expect this to not work:
Expect: nice error using this text. Predicted behavior: not-so-nice error using different text. For completeness, you should also test setting A to "http://example.com/" and B to "https://example.com/" -- that shouldn't be allowed. | |
Comment Actions
I don't know why unit is failing, trace gives no good clues (for me at least).
<<< [63] <exec> 16,838,142 us [2016-06-24 10:59:06] EXCEPTION: (CommandException) Command failed with error #2! COMMAND php '/var/www/html/dev/core/lib/phabricator/scripts/sql/manage_storage.php' upgrade --force --namespace 'phabricator_unittest_jaean24w7qcllqaihmknak6h' STDOUT Loading quickstart template... Applying patch 'phabricator:20160102.badges.award.sql'... Applying patch 'phabricator:20160119.project.1.silence.sql'... Applying patch 'phabricator:20160122.project.1.boarddefault.php'... Applying patch 'phabricator:20160124.people.1.icon.sql'... Applying patch 'phabricator:20160124.people.2.icondefault.sql'... Applying patch 'phabricator:20160128.repo.1.pull.sql'... Applying patch 'phabricator:20160202.board.1.proxy.sql'... Applying patch 'phabricator:20160202.ipv6.1.sql'... Applying patch 'phabricator:20160202.ipv6.2.php'... Applying patch 'phabricator:20160206.cover.1.sql'... Applying patch 'phabricator:20160208.task.1.sql'... Applying patch 'phabricator:20160208.task.2.sql'... Applying patch 'phabricator:20160208.task.3.sql'... Applying patch 'phabricator:20160212.proj.1.sql'... Applying patch 'phabricator:20160212.proj.2.sql'... Applying patch 'phabricator:20160215.owners.policy.1.sql'... Applying patch 'phabricator:20160215.owners.policy.2.sql'... ... (11,830 more bytes) ... STDERR (empty) at [<phutil>/src/future/exec/ExecFuture.php:361] arcanist(head=master, ref.master=2374403e8f80), corgi(head=master, ref.master=cbb6f678929c), phabricator(head=phame-https, ref.master=70463e8a16d4, ref.phame-https=6369b9e7e9d7), phutil(head=master, ref.master=51c179b4c000) #0 ExecFuture::resolvex() called at [<phutil>/src/future/exec/execx.php:17] #1 execx(string, string, string) called at [<phabricator>/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php:16] #2 PhabricatorStorageFixtureScopeGuard::__construct(string) called at [<phabricator>/src/infrastructure/testing/PhabricatorTestCase.php:174] #3 PhabricatorTestCase::newStorageFixture() called at [<phabricator>/src/infrastructure/testing/PhabricatorTestCase.php:69] #4 PhabricatorTestCase::willRunTestCases(array) called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:64] #5 PhutilUnitTestEngine::run() called at [<arcanist>/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php:147] #6 ArcanistConfigurationDrivenUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:167] #7 ArcanistUnitWorkflow::run() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1340] #8 ArcanistDiffWorkflow::runUnit() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1228] #9 ArcanistDiffWorkflow::runLintUnit() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:483] #10 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]
Comment Actions
Here's how you can figure that one out -- the issue is that the error is hiding down at the bottom of the output. So you can just run a similar command, like this:
$ ./bin/storage upgrade --force --namespace lsdnfklansfdlnas...where "lsdnfklansfdlnas" is a random string you type by mashing your keyboard. That produces a lot of output, then eventually fails with this error:
... Target Error lsdnfklansfdlnas_phame.phame_blog.domainFullURI Wrong Nullable Setting ...
See inline for two possible fixes.
Then you can clean up like this if you want:
$ ./bin/storage destroy --namespace lsdnfklansfdlnas| resources/sql/autopatches/20160623.phame.blog.fulldomain.1.sql | ||
|---|---|---|
| 3 | One fix is to remove NOT NULL, so the column is nullable. | |
| src/applications/phame/storage/PhameBlog.php | ||
| 50 | One fix is to remove this ?, so the column isn't nullable. | |
| src/applications/phame/storage/PhameBlog.php | ||
|---|---|---|
| 50 | If I remove it, I get a Schemata Error. Which is why I added it. | |
Comment Actions
phabricator_phame.phame_blog.domainFullURI Wrong Nullable Setting SCHEMATA ERRORS The schemata have errors (detailed above) which the adjustment workflow can not fix.
Comment Actions
Database changed mysql> show columns in phame_blog; +------------------+------------------+------+-----+---------+----------------+ | Field | Type | Null | Key | Default | Extra | +------------------+------------------+------+-----+---------+----------------+ | id | int(10) unsigned | NO | PRI | NULL | auto_increment | | phid | varbinary(64) | NO | UNI | NULL | | | name | varchar(64) | NO | | NULL | | | description | longtext | NO | | NULL | | | domain | varchar(128) | YES | UNI | NULL | | | configData | longtext | NO | | NULL | | | creatorPHID | varbinary(64) | NO | | NULL | | | dateCreated | int(10) unsigned | NO | | NULL | | | dateModified | int(10) unsigned | NO | | NULL | | | viewPolicy | varbinary(64) | YES | | NULL | | | editPolicy | varbinary(64) | YES | | NULL | | | mailKey | binary(20) | NO | | NULL | | | status | varchar(32) | NO | | NULL | | | profileImagePHID | varbinary(64) | YES | | NULL | | | headerImagePHID | varbinary(64) | YES | | NULL | | | subtitle | varchar(64) | NO | | NULL | | | parentDomain | varchar(128) | NO | | NULL | | | parentSite | varchar(128) | NO | | NULL | | | domainFullURI | varchar(128) | YES | | NULL | | +------------------+------------------+------+-----+---------+----------------+
Comment Actions
mysql> SHOW CREATE TABLE phabricator_phame.phame_blog; +------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | phame_blog | CREATE TABLE `phame_blog` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `phid` varbinary(64) NOT NULL, `name` varchar(64) COLLATE utf8mb4_bin NOT NULL, `description` longtext COLLATE utf8mb4_bin NOT NULL, `domain` varchar(128) COLLATE utf8mb4_bin DEFAULT NULL, `configData` longtext COLLATE utf8mb4_bin NOT NULL, `creatorPHID` varbinary(64) NOT NULL, `dateCreated` int(10) unsigned NOT NULL, `dateModified` int(10) unsigned NOT NULL, `viewPolicy` varbinary(64) DEFAULT NULL, `editPolicy` varbinary(64) DEFAULT NULL, `mailKey` binary(20) NOT NULL, `status` varchar(32) COLLATE utf8mb4_bin NOT NULL, `profileImagePHID` varbinary(64) DEFAULT NULL, `headerImagePHID` varbinary(64) DEFAULT NULL, `subtitle` varchar(64) COLLATE utf8mb4_bin NOT NULL, `parentDomain` varchar(128) COLLATE utf8mb4_bin NOT NULL, `parentSite` varchar(128) COLLATE utf8mb4_bin NOT NULL, `domainFullURI` varchar(128) COLLATE utf8mb4_bin DEFAULT NULL, PRIMARY KEY (`id`), UNIQUE KEY `phid` (`phid`), UNIQUE KEY `domain` (`domain`) ) ENGINE=InnoDB AUTO_INCREMENT=5 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin | +------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
Comment Actions
+------------------+------------------+------+-----+---------+----------------+
| Field | Type | Null | Key | Default | Extra |
+------------------+------------------+------+-----+---------+----------------+
...
| domainFullURI | varchar(128) | YES | | NULL | |
...
^^^^^^Your column is nullable, but the SQL patch says "NOT NULL". Easiest fix is likely to throw the column away:
ALTER TABLE phabricator_phame.phame_blog DROP domainFullURI;
...then re-apply the patch forcefully:
$ ./bin/storage upgrade --force --apply phabricator:20160623.phame.blog.fulldomain.1.sqlOne other thing you should test is whether creating a new blog works. It might fail ("no value for column domainFullURI!") -- if so, add setDomainFullURI('') to initializeNewBlog().
Comment Actions
- Cleaner SQL
| resources/sql/autopatches/20160623.phame.blog.fulldomain.1.sql | ||
|---|---|---|
| 3 | Just made this nullable. | |