Upgrade to Laravel 5.4 #227

m1guelpf · 2017-07-20T12:01:41Z

This pull request upgrades Symposium to Laravel 5.4

mattstauffer

Thanks for this!

mattstauffer · 2017-07-20T16:11:08Z

app/Providers/AppServiceProvider.php

@@ -20,8 +20,8 @@ class AppServiceProvider extends ServiceProvider
     */
    public function boot()
    {
-        \Blade::setRawTags('{{', '}}');
-        \Blade::setContentTags('{{{', '}}}');
+        // \Blade::setRawTags('{{', '}}');


If we're going to escape this, we need to make sure we change the output style everywhere.

mattstauffer · 2017-07-20T16:36:01Z

config/app.php

@@ -165,7 +165,7 @@
        'Collective\Html\HtmlServiceProvider',
        'Thomaswelton\LaravelGravatar\LaravelGravatarServiceProvider',
        'Thujohn\Twitter\TwitterServiceProvider',
-        'Maknz\Slack\SlackServiceProvider',
+        M1guelpf\Slack\SlackServiceProvider::class,


The other package is no longer supported, and is not Laravel5.4-compatible.

mattstauffer · 2017-07-20T18:12:52Z

resources/views/dashboard.blade.php

-                            No talks yet.
-                        </li>
-                    @endforelse
+                      @each('partials.talk-in-list', $talks, 'talk', 'partials.talk-in-list-empty')


too many spaces indent

mattstauffer · 2017-07-20T18:13:07Z

resources/views/partials/talk-in-list-empty.blade.php

@@ -0,0 +1 @@
+No talks yet.


Wouldn't this need an
around it?

mattstauffer · 2017-07-20T18:13:47Z

tests/AccountTest.php

@@ -2,11 +2,11 @@

 use App\User;
 use Laracasts\TestDummy\Factory;
-use MailThief\Testing\InteractsWithMail;
+// use MailThief\Testing\InteractsWithMail;


If we don't need this we can delete it

mattstauffer · 2017-07-20T18:14:28Z

tests/Api/DummyAuthorizer.php

+{
+    public $userId;
+
+    public function __construct()


Is constructor necessary?

mattstauffer · 2017-07-20T18:15:00Z

tests/PublicSpeakerProfileTest.php

@@ -5,12 +5,12 @@
 use Illuminate\Foundation\Testing\DatabaseMigrations;
 use Illuminate\Support\Facades\Session;
 use Laracasts\TestDummy\Factory;
-use MailThief\Testing\InteractsWithMail;
+// use MailThief\Testing\InteractsWithMail;


same as in previous; if we don't need it then you can delete it

mattstauffer · 2017-07-20T18:15:15Z

tests/SubmissionTest.php

+//         $this->assertEquals(0, $conference->submissions->count());
+//         $this->assertFalse($conference->submissions->contains($revision));
+//     }
+// }


? This is not OK

I temporally commented it out because it was failing for an unknown reason.

mattstauffer · 2017-07-20T18:15:36Z

tests/Api/DummyAuthorizer.php

+
+namespace Tests\Api;
+
+class DummyAuthorizer extends Authorizer


Why introduce a class that's never used?

mattstauffer · 2017-07-20T18:16:05Z

app/Handlers/Events/SlackSubscriber.php

@@ -3,7 +3,7 @@
 namespace App\Handlers\Events;

 use Illuminate\Support\Facades\App;
-use Maknz\Slack\Facades\Slack;
+use Slack;


Importing façades with their FQCN is an intentional decision.

m1guelpf · 2017-07-20T19:04:39Z

resources/views/partials/talk-in-list.blade.php

@@ -1,4 +1,5 @@
-<div class="pull-right">


m1guelpf · 2017-07-20T20:02:01Z

@mattstauffer I think I need help with the failing/commented out tests. The feature works in my browser, but fails in the tests...

imjohnbon · 2017-08-10T16:55:18Z

@m1guelpf Hi Miguel, thank you for all of your work on this upgrade! I took some time this morning to review the failing/commented out tests that you noted and was able to get to the bottom of it. I wanted to take a moment to outline what the issues were, as they were definitely a bit obscure.

The majority of the tests that were causing issues were in the tests/SubmissionTest.php file. I noticed that each test that was failing was failing because of a line like this:

$this->assertTrue($conference->submissions->contains($revision));

The error message was saying that it failed to "assert that false is true", meaning that the contains method on the collection was failing to find the model within the collection. This was especially confusing because when I dd()'d $conference->submissions in the terminal, I could see that the $revision clearly was inside the collection, which made it appear that the contains method itself was broken. That seemed hard to believe, but made sense when I dug deeper.

When you call contains on an Eloquent collection, and you pass in a model, here's the code that eventually gets run (in Laravel 5.4):

return parent::contains(function ($model) use ($key) {
    return $model->is($key);
});

You can see that on line 87 of Illuminate\Database\Eloquent\Collection.

Alternatively, here's the code that gets run when you call contains on an Eloquent collection when you pass in a model in Laravel 5.3:

return parent::contains(function ($model) use ($key) {
    return $model->getKey() == $key;
});

This one you can see on line 83 of Illuminate\Database\Eloquent\Collection.

As you can tell, in 5.3 contains does a basic check for the models key (usually the id) within the collection. But in 5.4 it runs an is method on the model instead. Here's what that is method looks like:

public function is(Model $model)
{
    return $this->getKey() === $model->getKey() &&
        $this->getTable() === $model->getTable() &&
        $this->getConnectionName() === $model->getConnectionName();
}

The is method checks if a couple things are equal (the key, the table, and the connection name), and that's where the problem comes in. When you create a factory like this (like we do in every test):

$revision = Factory::create('talkRevision');

and then call getConnectionName on it like this:

$revision->getConnectionName()

You're going to see it returns null. In order to be able to successfully get the connection name from the model, we need to grab a fresh copy of it from the database like so:

$revision = $revision->fresh();

Now if you call getConnectionName on $revision you're going to see it returns something like testing (if it's being run during a test).

So, that was the underlying issue here. We were passing an Eloquent model into the contains method right after it was created by a Factory, which was causing getConnectionName to return null and thus, fail the is check. Prior to 5.4, without the is check, this test passed just fine.

The one other test that was failing was the user_can_reset_their_password_from_email_link test inside tests/AccountTest.php. The test was assuming that after we reset our password, we would be sent back to the dashboard page, however in actuality, we were being sent back to the reset password page. After dd()'ing the response in the terminal, I realized that the test was failing because the password reset token was invalid, thus triggering a validation error and taking us back to the password reset page.

Anyway, here's the relevant portion of the code from that test:

$reset_token = DB::table('password_resets')->where('email', $user->email)->pluck('token')->first();
        
$this->visit('/password/reset/' . $reset_token)

...

As you can see, we grab the reset token from the database, and then we pass that into the reset password URL for the test. However, after a bit of Googling, I found out that there was an undocumented change in how password reset tokens worked between 5.3 and 5.4. You can read more about it here, but essentially the tokens are now hashed before they are stored in the database. However, the un-hashed version is still what goes in the URL in the email that gets sent to the user.

So, essentially, we were trying to use the hashed version of the token in the URL (because we got it from the database directly), when what we really needed was the un-hashed version. This led me down a bit of a rabbit hole of trying to figure out how to get that un-hashed version (since it's not stored anywhere in the DB). Luckily, I eventually found this Stackoverflow that held the answer. By using a Notification fake in the test, we can get the un-hashed version via a callback, and use it successfully in our test.

If you have any questions about any of this, I'd be happy to answer. Thanks again for all the hard work!

m1guelpf · 2017-08-10T16:58:00Z

@imjohnbon So this is ready now?

mattstauffer · 2017-08-10T18:29:40Z

@imjohnbon thanks for your hard work sussing that out!

@m1guelpf nope, not ready. John is going to move the Slack notifications over to native Laravel notifications and then I'll review the whole thing again.

imjohnbon · 2017-08-10T19:43:49Z

@mattstauffer Thanks! The Slack notifications have now been fully converted over to native Laravel notifications.

m1guelpf · 2017-09-06T21:53:26Z

@mattstauffer ping

mattstauffer · 2017-09-22T01:44:11Z

Upgrading Vue and Axios etc. broke the Vue content. Currently deciding whether to upgrade it or just downgrade the dependencies.

mattstauffer · 2017-09-22T02:04:24Z

Ugh. Also this requires 7.1 and symposium is currently hosted on 7.0.

m1guelpf · 2017-09-22T06:01:42Z

@mattstauffer Would you accept another PR for 5.5?

mattstauffer · 2017-09-22T17:26:46Z

@m1guelpf Sure! I'll try to get this on a 7.1 server soon. Thanks!

mattstauffer · 2017-10-06T14:20:58Z

@m1guelpf it's now on a 7.1 server, so you're welcome to PR 5.5 if you'd like. Thanks!

m1guelpf · 2017-10-06T14:23:59Z

@mattstauffer I've submitted lucadegasperi/oauth2-server-laravel#777, but the project is no longer mantained, so I guess the first thing to do would be switching to Passport.

mattstauffer requested changes Jul 20, 2017

View reviewed changes

m1guelpf commented Jul 20, 2017

View reviewed changes

resources/views/partials/talk-in-list.blade.php

@@ -1,4 +1,5 @@

<div class="pull-right">

Copy link

Contributor Author

m1guelpf Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Upgrade to Laravel 5.4

0c75cda

imjohnbon added 4 commits August 10, 2017 11:42

Uncomment tests

b03bcda

Load fresh revisions from DB

f708540

Get token from notification fake instead of DB

2282a26

Update composer.lock

93aa794

Cleanup extra spaces

3cf81d9

imjohnbon added 3 commits August 10, 2017 14:47

Remove slack package

f2dd6a3

Use built in notifications for slack alerts

9ae3d4c

Cleanup spacing

494cb32

mattstauffer merged commit 494cb32 into tighten:master Sep 22, 2017

andrewmile mentioned this pull request Oct 27, 2021

Manually setting Blade syntax to the old way means you can't use blade comments #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Laravel 5.4 #227

Upgrade to Laravel 5.4 #227

m1guelpf commented Jul 20, 2017 •

edited

Loading

mattstauffer left a comment

mattstauffer Jul 20, 2017

m1guelpf Jul 20, 2017

mattstauffer Jul 20, 2017

m1guelpf Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

m1guelpf Jul 20, 2017

mattstauffer Jul 20, 2017

mattstauffer Jul 20, 2017

m1guelpf Jul 20, 2017

m1guelpf commented Jul 20, 2017

imjohnbon commented Aug 10, 2017 •

edited

Loading

m1guelpf commented Aug 10, 2017

mattstauffer commented Aug 10, 2017

imjohnbon commented Aug 10, 2017

m1guelpf commented Sep 6, 2017

mattstauffer commented Sep 22, 2017

mattstauffer commented Sep 22, 2017

m1guelpf commented Sep 22, 2017

mattstauffer commented Sep 22, 2017

mattstauffer commented Oct 6, 2017

m1guelpf commented Oct 6, 2017


		namespace Tests\Api;

		class DummyAuthorizer extends Authorizer

Upgrade to Laravel 5.4 #227

Upgrade to Laravel 5.4 #227

Conversation

m1guelpf commented Jul 20, 2017 • edited Loading

mattstauffer 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

m1guelpf commented Jul 20, 2017

imjohnbon commented Aug 10, 2017 • edited Loading

m1guelpf commented Aug 10, 2017

mattstauffer commented Aug 10, 2017

imjohnbon commented Aug 10, 2017

m1guelpf commented Sep 6, 2017

mattstauffer commented Sep 22, 2017

mattstauffer commented Sep 22, 2017

m1guelpf commented Sep 22, 2017

mattstauffer commented Sep 22, 2017

mattstauffer commented Oct 6, 2017

m1guelpf commented Oct 6, 2017

m1guelpf commented Jul 20, 2017 •

edited

Loading

imjohnbon commented Aug 10, 2017 •

edited

Loading