FB-113 Site settings in DB - stage 1 #2460

fvovan · 2016-12-12T11:16:11Z

We need to split it into 2 parts in order migrate existing configuration properly.
This PR will contain schema changes, model classes and other functionality to allow migration, example application data and settings editor.
The second one will actually use these settings

fvovan · 2016-12-12T15:10:26Z

I'm not quite sure I chose best naming conventions here. CM_Site_SiteSettings looks similar to existing CM_Site_SiteFactory but may be it's not the best option.
The same is for configuration field of table. Besides it turned out we can't avoid using autoincrement ID value for models, so siteId was created. Also we discussed classType column creating, but now I doubt how it could be used considering CM_Site_Abstract->getId() is equal by definition to CM_Site_Abstract->getType().

@njam , @tomaszdurka could you have a look please while i'm preparing the second part?

njam

Some small notes.

Let's discuss on a call, will send invite.

njam · 2016-12-14T08:07:29Z

library/CM/Site/Abstract.php

+     * @param $type
+     * @return string|null
+     */
+    public static function findClassName($type) {


Seems to be unused

njam · 2016-12-14T08:30:45Z

library/CM/Site/SiteSettings.php

+    /**
+     * @return int
+     */
+    public function getConfigurationSize() {


njam · 2016-12-14T08:31:55Z

library/CM/Site/SiteSettings.php

+    /**
+     * @return string|null
+     */
+    public function findSiteClassName() {


please see comment for CM_Site_Abstract::findClassName()

njam · 2016-12-14T08:32:59Z

library/CM/Site/SiteSettings.php

+        if (!$this->_has('configuration')) {
+            return CM_Params::factory([]);
+        }
+        $paramsEncoded = CM_Params::jsonDecode($this->_get('configuration'));


Use CM_Util::jsonDecode() instead

njam · 2016-12-14T08:33:15Z

library/CM/Site/SiteSettings.php

+     * @param CM_Params $configuration
+     */
+    public function setConfiguration(CM_Params $configuration) {
+        $this->_set('configuration', CM_Params::jsonEncode($configuration->getParamsEncoded()));


Use CM_Util::jsonEncode() instead

njam · 2016-12-14T08:33:23Z

library/CM/Site/SiteSettings.php

+
+    protected function _getSchema() {
+        return new CM_Model_Schema_Definition([
+            'siteId'        => ['type' => 'int', 'optional' => true],


Why is it optional?

njam · 2016-12-14T08:33:39Z

library/CM/Site/SiteSettings.php

+    protected function _getSchema() {
+        return new CM_Model_Schema_Definition([
+            'siteId'        => ['type' => 'int', 'optional' => true],
+            'name'          => ['type' => 'string', 'optional' => true],


Make the name mandatory?

njam · 2016-12-14T08:34:23Z

library/CM/Site/SiteSettings.php

+     * @param int $siteId
+     * @return CM_Site_SiteSettings|null
+     */
+    public static function findBySiteId($siteId) {


How about keeping the same ID values for "site" and "site-settings"?
Then this would not be needed?

njam · 2016-12-14T08:34:36Z

library/CM/Util.php

@@ -592,6 +592,27 @@ public static function jsonDecode($value) {

    /**
     * @param string $value
+     * @return bool
+     */
+    public static function jsonIsValid($value) {


fvovan · 2016-12-23T07:20:10Z

this was simplified.
@njam please review

njam

For some functions I couldn't find usages. Maybe they will be used later? It seems the PR for stage 2 is not up to date, so I wasn't able to tell.

njam · 2016-12-31T17:43:06Z

library/CM/Site/Abstract.php

+     */
+    public static function getAll() {
+        $siteList = array();
+        foreach (CM_Config::get()->CM_Site_Abstract->types as $className) {


iirc we wanted this to use the database table, so that we could potentially have database-only sites in the future?

yes, you're right, but now we need to keep it in order to perform settings migration.

njam · 2016-12-31T17:47:20Z

library/CM/Site/Abstract.php

+     * @param $type
+     * @return string|null
+     */
+    public static function findClassName($type) {


This function is only used in findSiteClassName(), which could be using just get_class() and is itself unused. Can this be removed?

Hmm. the thing is we need (well, actually it's not strictly need) to get site class name only by CM_Site_SiteSetting::type_id.
we don't have site instance there to use get_class(). But on the other hand the only usage is admin UI we can omit it there and rather output another text label.

njam · 2016-12-31T17:48:08Z

library/CM/Site/SiteSettings.php

+     * @return CM_Params
+     */
+    public function getConfiguration() {
+        if (!$this->_has('configuration')) {


Is this necessary? The field is mandatory, so it should always be present?

Ahh. good catch

njam · 2016-12-31T17:49:16Z

library/CM/Site/SiteSettings.php

+     * @param string $key
+     * @param string $value
+     */
+    public function upsertConfigurationValue($key, $value) {


What is the use case for this?

Saving settings values in forms. Besides originally inserting of settings key was supported, but actually it works the same.

njam · 2016-12-31T17:52:44Z

library/CM/Site/SiteSettings.php

+
+    protected function _getSchema() {
+        return new CM_Model_Schema_Definition([
+            'siteType'      => ['type' => 'int', 'optional' => true],


Why is it optional?

it's connected to the fact that sometimes we create settings before even have any site class. Mostly it happens in tests. Yep, code intended only for tests is obviously a code smell. And i'm not quite sure how best to proceed here. In fact the only purpose of cm_site_setting.siteId is mapping for CM_Site_Abstract::getAll(). That's why maybe it would be a good idea to have "not persisted setting" class. So theoretically we'll be able to create sites with settings which are not in the DB.

njam · 2016-12-31T17:53:20Z

library/CM/Site/SiteSettings.php

+            $configuration = CM_Params::factory([]);
+        }
+        $siteSettings = new self();
+        $siteSettings->_set([


Let's use the setters instead, this way we can avoid duplicating some code ( jsonEncode()).

njam · 2016-12-31T17:58:27Z

resources/db/structure.sql

+  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
+  `siteType` int(10) unsigned DEFAULT NULL,
+  `name` varchar(32) NOT NULL,
+  `configuration` varchar(2000) NOT NULL,


I think we should make it larger. We might potentially store a lot of strings in here. And since this table is small and this field is not indexed it doesn't matter for performance. 65535?

I set it to 21800. (maximum value for utf-8 is 21844 but for some reason it breaks tests)

njam · 2016-12-31T17:58:50Z

resources/db/structure.sql

+  `configuration` varchar(2000) NOT NULL,
+  PRIMARY KEY (`id`),
+  UNIQUE KEY `siteType` (`siteType`)
+) ENGINE=MyISAM DEFAULT CHARSET=utf8;


Should we use InnoDB for this new table where performance considerations don't matter?

njam · 2016-12-31T17:58:58Z

tests/library/CM/Paging/SiteSettings/AllTest.php

@@ -0,0 +1,19 @@
+<?php
+class CM_Paging_SiteSettings_AllTest extends CMTest_TestCase {


njam · 2016-12-31T18:00:01Z

tests/library/CM/Paging/SiteSettings/AllTest.php

+        $settingsAdded = CM_Site_SiteSettings::create(1, 'Baz', CM_Params::factory(['bar' => 'foo']));
+        $paging = new CM_Paging_SiteSettings_All();
+        $this->assertSame(1, $paging->getCount());
+        $settings = $paging->getItem(0);


A more concise way of asserting pagings is:

$this->assertEquals([$item1, $item2], $paging);

njam · 2017-01-02T16:36:26Z

I think it would be good to implement getters and setters on the site class(es) for the different fields, e.g. getSupportEmail(), and have a strict schema for them.
Doing that in the SiteSettings model would be great, as we could re-use the existing schema mechanism, but it would mean that we'd need to have separate SiteSettings models in SK and in CM.

Alternatively we could just implement getters and setters with casting on the Site model, which would access the data on the SiteSettings with generic functions accessing the data bag.
@tomaszdurka @fvovan wdyt?

# Conflicts: resources/db/update/76.php

njam · 2017-01-18T12:13:58Z

As discussed: maybe it's better to make the Site extend Model so we can reuse schema and persistence functionality.
Vladimir will prepare a new branch with how the final logic could look in CM. Then we can compare it and decide which way to go.

fvovan · 2017-06-27T11:18:02Z

was done in https://upsource.cargomedia.ch/cm/review/CM-CR-29

fvovan added 4 commits December 12, 2016 10:21

just rearrange methods according codestyle

8582bd0

model, pagings and db schema

6c963ee

CM_Params helper

15924c2

add jsonValidate

c31305d

fvovan added the selfmade label Dec 12, 2016

fvovan self-assigned this Dec 12, 2016

fvovan added the slack label Dec 12, 2016

njam mentioned this pull request Dec 12, 2016

FB-113 Site settings in DB - stage 2 #2428

Closed

refactor jsonIsValid()

2ced04f

njam suggested changes Dec 14, 2016

View reviewed changes

fvovan added 10 commits December 15, 2016 17:18

don't use deprecated methods

be21188

add upsertConfValue()

b5e9f8c

try mandatory name

f479d06

adjust factory

1c91037

Merge branch 'master' of github.com:cargomedia/CM into site_settings_1

d1937c7

siteType refactor

5fbc3a8

Merge branch 'master' of github.com:cargomedia/CM into site_settings_1

a0194e2

remove JsonIsValid

1fa2c1a

remove configuration size

a32348e

fix test

35e32fc

njam suggested changes Dec 31, 2016

View reviewed changes

fvovan added 3 commits January 10, 2017 11:21

Merge branch 'master' of github.com:cargomedia/CM into site_settings_1

d1210f1

adjustments part 1

94a2cdf

Merge branch 'master' of github.com:cargomedia/CM into site_settings_1

4a00d3b

# Conflicts: resources/db/update/76.php

njam changed the title ~~Site settings in DB - stage 1~~ FB-113 Site settings in DB - stage 1 Feb 26, 2017

njam unassigned fvovan Mar 20, 2017

fvovan closed this Jun 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FB-113 Site settings in DB - stage 1 #2460

FB-113 Site settings in DB - stage 1 #2460

fvovan commented Dec 12, 2016 •

edited

Loading

fvovan commented Dec 12, 2016

njam left a comment

njam Dec 14, 2016

njam Dec 14, 2016

njam Dec 14, 2016

fvovan Jan 17, 2017

njam Dec 14, 2016

njam Dec 14, 2016

njam Dec 14, 2016

njam Dec 14, 2016

njam Dec 14, 2016

njam Dec 14, 2016

fvovan commented Dec 23, 2016

njam left a comment

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

njam Dec 31, 2016

fvovan Jan 17, 2017

njam Dec 31, 2016

njam Dec 31, 2016

njam Dec 31, 2016

njam commented Jan 2, 2017

njam commented Jan 18, 2017

fvovan commented Jun 27, 2017

		@@ -0,0 +1,19 @@
		<?php
		class CM_Paging_SiteSettings_AllTest extends CMTest_TestCase {

FB-113 Site settings in DB - stage 1 #2460

FB-113 Site settings in DB - stage 1 #2460

Conversation

fvovan commented Dec 12, 2016 • edited Loading

fvovan commented Dec 12, 2016

njam 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

fvovan commented Dec 23, 2016

njam 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

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

njam commented Jan 2, 2017

njam commented Jan 18, 2017

fvovan commented Jun 27, 2017

fvovan commented Dec 12, 2016 •

edited

Loading