Remove the remains of kohana #1841

rjmackay · 2017-07-03T02:24:57Z

This pull request makes the following changes:

Stop bootstrapping Kohana
Remove all kohana related composer deps
Update Media to work without kohana media

Test checklist:

bin/behat

Refs #1372

Ping @ushahidi/platform

willdoran

Looking cool

willdoran · 2017-07-04T15:34:17Z

app/Tools/LumenMailer.php

+
+    public function send($to, $type, Array $params = null)
+    {
+        $method = "send" . Str::ucfirst($type);


Could we add a comment about available types?

willdoran · 2017-07-04T15:36:14Z

app/Tools/LumenMailer.php

+        }
+    }
+
+    protected function sendResetpassword($to, $params)


In the end, are we going to have subclass of this for Notifications, Reset etc or will they just live as function within?

I'm not sure yet. I suspect we might want to replace it entirely with something much closer to Laravel's Mailables

willdoran · 2017-07-04T15:44:15Z

composer.json

-        "barryvdh/laravel-cors": "^0.9.2"
+        "barryvdh/laravel-cors": "^0.9.2",
+        "illuminate/mail": "5.4.*",
+        "mockery/mockery": "^0.9.9"


This is a great name for a mock lib :D

If only it didn't break everything!

willdoran · 2017-07-04T15:45:18Z

config/mail.php

+    |
+    */
+
+    'from' => ['address' => env('MAIL_ADDRESS'), 'name' => env('MAIL_NAME')],


Do these derive from the email used to sign up to cloud?

Not in this instance, in the mailer they load from the site name + site email set in settings.

willdoran · 2017-07-04T15:59:49Z

src/App/Multisite.php

 		}

 		// Set new database config
-		$config = Kohana::$config->load('database')->default;
+		$config = Repository\OhanzeeRepository::getDefaultConfig();
+		Kohana::$config->load('database')->default;


Is this line needed?

rjmackay · 2017-12-21T08:44:35Z

@willdoran this should be ready for review now

rjmackay · 2017-12-21T08:45:55Z

app/Providers/AppServiceProvider.php

+        });
+
+        // Intercom config settings
+        $di->set('site.intercomAppToken', function () use ($di) {


@willdoran I don't know that this is really used anywhere anymore?

rjmackay · 2017-12-21T08:46:50Z

config/media.php

+ * Media Config
+ */
+
+return array(


Pretty sure this could be combined with CDN config, but that all needs some rationalization now we could use Laravel Filesystem

We should generally consider how we should logically group configs

rjmackay · 2017-12-21T08:47:46Z

src/App/Formatter/Media.php

 		} else {
-            // URL::site or Media::uri already encodes the path properly, skip the path wrangling seen above
-			return \URL::site(\Media::uri($this->getRelativePath() . $value), \Request::current());
+			return url(config('cdn.local.media_upload_dir') . '/' . $path); // @todo load from config


I'm sure theres a nicer way to do this once we use laravel filesystem

- Stop bootstrapping kohana - Stop using Arr - Fix refs to DOCROOT and use base_path() instead - Move DB and CDN config into config/

…age()

getDefaultConfig() method doesn't exist

rowasc · 2018-01-23T19:59:37Z

app/Providers/AppServiceProvider.php

+        $this->app->configure('multisite');
+        $this->app->configure('ohanzee-db');
+
+        $this->registerServicesFromAura();


why does it matter that it is using AuraDI to register usecase and repository.message?

rowasc · 2018-01-23T20:04:18Z

app/Providers/AppServiceProvider.php

-            return $site['private']
-                and $features['private']['enabled'];
+        // Multisite db
+        // Move multisite enabled check to class and move to src/App


when we talk about multisite install, is that the way we have the setup for ushahidi.io clients?

I think that's correct, yes

willdoran

My comments are very general. I don't see any obvious issue.

My suggestion is that as part of wrapping this a doc is delivered that covers what was surfaced as important future concerns. And what next steps might be for further piece by piece upgrades of the API.

willdoran · 2018-01-24T17:15:03Z

app/Http/Controllers/RESTController.php

+            if ($type === 'jsonp') {
+                $response->withCallback($request->input('callback'));
+                // Prevent Opera and Chrome from executing the response as anything
+                // other than JSONP, see T455.


could this be a link

Ugh its a reference to phabricator WAT

willdoran · 2018-01-24T17:15:52Z

app/Providers/AppServiceProvider.php

-            return $site['private']
-                and $features['private']['enabled'];
+        // Multisite db
+        // Move multisite enabled check to class and move to src/App


I think that's correct, yes

willdoran · 2018-01-24T17:16:17Z

app/Providers/AppServiceProvider.php

+        // Multisite db
+        // Move multisite enabled check to class and move to src/App
+        $di->set('site', function () use ($di) {
+            $site = '';


Can we add a todo to make this a logical default?

willdoran · 2018-01-24T17:17:04Z

bootstrap/lumen.php

@@ -15,7 +15,7 @@
    realpath(__DIR__.'/../')
 );

-// $app->withFacades();
+$app->withFacades();


Did you say this needs testing?

Sorta - the branch is green so its tested :)

willdoran · 2018-01-24T17:17:33Z

config/media.php

+ * Media Config
+ */
+
+return array(


We should generally consider how we should logically group configs

willdoran · 2018-01-24T17:18:58Z

config/media.php

+
+	// Maximum file upload size in bytes. Remember this figure should not be larger
+	// than the maximum file upload size set on the server. 1Mb by default.
+	'max_upload_bytes' => '1048576',


Can we add a todo that setting bytes is gross and should be made more logical somehow? I imagine this is something people may want to readily configure, which means they have to either know base 10 to an even number off by heart or google it.

rjmackay added the Stage: In Review label Jul 3, 2017

rjmackay force-pushed the lumen-remove-kohana branch 2 times, most recently from 02c1eaa to 844768e Compare July 3, 2017 03:41

rjmackay changed the title ~~Lumen remove kohana~~ Lumen remove kohana [WIP] Jul 4, 2017

willdoran reviewed Jul 4, 2017

View reviewed changes

rjmackay force-pushed the lumen-remove-kohana branch from 844768e to 65f9040 Compare July 4, 2017 23:02

rjmackay mentioned this pull request Jul 5, 2017

Migrate to Lumen #1663

Merged

13 tasks

rjmackay force-pushed the lumen-remove-kohana branch from 65f9040 to 2062e61 Compare July 5, 2017 00:13

willdoran added Stage: In Development and removed Stage: In Review labels Sep 18, 2017

rjmackay self-assigned this Oct 19, 2017

rjmackay removed the In Development label Oct 24, 2017

rjmackay force-pushed the lumen branch 2 times, most recently from 027fd2b to 05ae30f Compare October 30, 2017 08:26

rjmackay force-pushed the lumen-remove-kohana branch 3 times, most recently from e722992 to bbce9c1 Compare December 21, 2017 01:50

chore: Remove all kohana related dependencies

f666cae

rjmackay force-pushed the lumen-remove-kohana branch from bbce9c1 to bbcfe13 Compare December 21, 2017 04:21

rjmackay changed the title ~~Lumen remove kohana [WIP]~~ Lumen remove kohana Dec 21, 2017

rjmackay requested a review from rowasc December 21, 2017 08:44

rjmackay commented Dec 21, 2017

View reviewed changes

rjmackay added 5 commits December 21, 2017 22:00

refactor: Stop bootstrapping kohana and fixup broken kohana deps

5221ad1

- Stop bootstrapping kohana - Stop using Arr - Fix refs to DOCROOT and use base_path() instead - Move DB and CDN config into config/

chore: Move media for tests out of application/media

381756b

fix: Remove kohana dependency from JSON formatter

1b58122

refactor: Remove kohana refs in media formatter

43b1ddf

fix: Remove check for SYSPATH from validation lang files

7034f15

rjmackay added 9 commits December 21, 2017 22:00

chore: Git ignore media uploads

c11471e

refactor: remove application/

124eb58

refactor: Update media formatter and config work without kohana-media

ef2e1ed

style: Fix lint issues in in AppServiceProvider

73c28f7

refactor: Update Notification command to use trans() not Kohana::mess…

66acf80

…age()

refactor: Log using facade not kohana

ef94959

fix: Use config() in multisite not Kohana::$config

8c37b77

fix: Remove stray use Kohana statement

d356517

docs: Remove Kohana refs in comments

457f93e

rjmackay force-pushed the lumen-remove-kohana branch from 51478f1 to 457f93e Compare December 21, 2017 09:01

rjmackay changed the title ~~Lumen remove kohana~~ Remove the remains of kohana Dec 21, 2017

rjmackay added 2 commits January 23, 2018 17:11

refactor: Start splitting DI into sections or moving into Init.php

926b7db

fix: Multisite needs to grab ohanzee-db config

d4f76db

getDefaultConfig() method doesn't exist

rowasc reviewed Jan 23, 2018

View reviewed changes

rjmackay added 2 commits January 24, 2018 10:52

refactor: clean out unused code in RESTController

03d80aa

chore: Enable facades

5b2137e

willdoran reviewed Jan 24, 2018

View reviewed changes

docs: Add todos and remove old comment refs

a7d8bb8

rjmackay merged commit 77e0530 into lumen Jan 25, 2018

rjmackay deleted the lumen-remove-kohana branch January 25, 2018 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the remains of kohana #1841

Remove the remains of kohana #1841

rjmackay commented Jul 3, 2017 •

edited

Loading

willdoran left a comment

willdoran Jul 4, 2017

willdoran Jul 4, 2017

rjmackay Jul 4, 2017

willdoran Jul 4, 2017

rjmackay Jul 4, 2017

willdoran Jul 4, 2017

rjmackay Jul 4, 2017

willdoran Jul 4, 2017

rjmackay commented Dec 21, 2017

rjmackay Dec 21, 2017

rjmackay Dec 21, 2017

willdoran Jan 24, 2018

rjmackay Dec 21, 2017

rowasc Jan 23, 2018 •

edited

Loading

rowasc Jan 23, 2018

willdoran Jan 24, 2018

willdoran left a comment

willdoran Jan 24, 2018

rjmackay Jan 25, 2018

willdoran Jan 24, 2018

willdoran Jan 24, 2018

willdoran Jan 24, 2018

rjmackay Jan 25, 2018

willdoran Jan 24, 2018

willdoran Jan 24, 2018

Remove the remains of kohana #1841

Remove the remains of kohana #1841

Conversation

rjmackay commented Jul 3, 2017 • edited Loading

willdoran 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

rjmackay commented Dec 21, 2017

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

rowasc Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdoran 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

rjmackay commented Jul 3, 2017 •

edited

Loading

rowasc Jan 23, 2018 •

edited

Loading