Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Laravel upgrade checklist #5473

Closed
filipac opened this issue Jan 27, 2021 · 8 comments
Closed

Laravel upgrade checklist #5473

filipac opened this issue Jan 27, 2021 · 8 comments

Comments

@filipac
Copy link
Contributor

filipac commented Jan 27, 2021

I've successfully upgraded a fresh OctoberCMS to use Laravel ^8.0 ( 8.25.0 as of speaking) from Laravel 6 we currently use.
I am sure there is no desire from core team to upgrade to non-lts version for now, but I just want to document what changes should we make to have it working:

  • require laravel components ^8.0
  • upgrade symfony/yaml in october/rain to ^5.1
  • classes in October\Rain\Database\Query\Grammars give an exception because the implementation is not as in parent class. Changed the compileUpsert functions to use Illuminate\Database\Query\Builder instead of October\Rain\Database\QueryBuilder and it worked.
  • October\Rain\Events\Dispatcher needed an update especially in listen method cause the signature changed
  • in Foundation/Bootstrap/LoadEnvironmentVariables we need to change the way Dotenv::create is called (first parameter now should be \Illuminate\Support\Env::getRepository(), the other 2 parameters remain the same.
  • in Exception/Handler we should change \Exception to \Throwable.
  • backend/models/brandsetting/fields.yaml needs an update in availableColors keys because the new Symfony Yaml parser sees colors as a comment and we get an exception of invalid Yaml. Did not check other yaml files if they include # but we should do that.
  • Laravel 9 will require at least PHP 7.4

Otherwise, everything seems to work so far on Laravel 8.0. Not sure what the changes will be in 9.0 which is scheduled for September this year, this will be the next LTS. But from what I've seen on Github, nothing too breaking, just upgrading some components.

I think we should be prepared to upgrade anytime and maybe sometime in the future try to mirror the latest supported version of Laravel (not only LTS) since we will now have only one per year. If we keep piling up versions to upgrade, it becomes a nightmare job. Since October is evergreen and we now have some kind of versioning, I do not see a problem mirroring latest Laravel, especially since we have a wrapper around the framework and not much of our/plugin's code needs to be updated.

Is this repo the best place to have this checklist registered, or is https://github.com/octoberrain/meta more suited?

Just want to help and stay on track with the great Laravel releases, this would be a strong point for us (see Statamic which always supports latest versions of Laravel).

@mjauvin
Copy link
Contributor

mjauvin commented Jan 27, 2021

Keep refining this until you get to L9 LTS and you will make friends! 😄

@filipac
Copy link
Contributor Author

filipac commented Jan 27, 2021

Yeah we have time to be prepared now, will try tomorrow to update to development branch of 9.0 to see what further changes we need to do.

We should be fast this time to upgrade, not like last time haha. Being on the latest version fast would gain traction.

@bennothommo
Copy link
Contributor

Nice work @filipac - great job.

Re:

upgrade symfony/yaml in october/rain to ^5.1

and

backend/models/brandsetting/fields.yaml needs an update in availableColors keys because the new Symfony Yaml parser sees colors as a comment and we get an exception of invalid Yaml. Did not check other yaml files if they include # but we should do that.

We've been discussing in #5271 about running a pre-processor for Yaml files to gain compatibility with the later versions of symfony/yaml, as all our docs and I would say the vast majority of plugin Yaml files will be using a syntax that is no longer compatible, especially when it comes to the version number keys. This would have to be implemented before we can consider upgrading the Yaml library.

Otherwise, everything looks relatively graceful.

Did you happen to run the unit tests and confirm that everything still passed?

@filipac
Copy link
Contributor Author

filipac commented Jan 27, 2021

@bennothommo i've run them now. Here's what I found so far

  • some tests fail due to invalid yaml in fixtures
  • we have to register in the container a new service "mail.manager" - \Illuminate\Mail\MailManager should be extended to suit October's needs
  • the mail system is a bit changed, the config file from legacy version still works but the Mail facade now points to the MailManager class.
  • Declaration of October\Rain\Database\Pivot::setKeysForSaveQuery(Illuminate\Database\Eloquent\Builder $query) should be compatible with Illuminate\Database\Eloquent\Model::setKeysForSaveQuery($query)
  • 1) CmsExceptionTest::testCmsExceptionPhp Error: Class 'Symfony\Component\Debug\Exception\FatalErrorException' not found - the new alternative is \Symfony\Component\ErrorHandler\Error\FatalError
  • \October\Rain\Exception\ExceptionBase methods setMask and applyMask should receive \Throwable
  • same in \Cms\Classes\CmsException - replace exception with \Throwable
  • AttachOneModelTest fails because the signature of UploadedFile changed and file_size is no longer required
  • UploadedFile method getClientSize no longer exists, change to getSize in \October\Rain\Database\Attach\File
  • MassAsignment no longer works because in laravel it was moved to Concerns\GuardsAttributes and we do not apply it on our model;

After changing the above, the remaning 2 failures and some errors from phpunit are:

There were 2 errors:

1) BelongsToManyModelTest::testSetRelationValue
Illuminate\Database\QueryException: SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: database_tester_authors_roles.author_id, database_tester_authors_roles.role_id (SQL: insert into "database_tester_authors_roles" ("author_id", "role_id") values (1, 3))

vendor/laravel/framework/src/Illuminate/Database/Connection.php:678
vendor/laravel/framework/src/Illuminate/Database/Connection.php:638
vendor/laravel/framework/src/Illuminate/Database/Connection.php:472
vendor/laravel/framework/src/Illuminate/Database/Connection.php:424
vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2838
rain/src/Database/QueryBuilder.php:304
rain/src/Database/Relations/BelongsToMany.php:136
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:175
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:112
rain/src/Database/Relations/BelongsToMany.php:339
rain/src/Support/Traits/Emitter.php:120
rain/src/Database/Model.php:168
rain/src/Events/Dispatcher.php:397
rain/src/Events/Dispatcher.php:242
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:189
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:898
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:869
rain/src/Database/Model.php:798
rain/src/Database/Model.php:831
tests/unit/plugins/database/BelongsToManyModelTest.php:57

Caused by
Doctrine\DBAL\Driver\PDO\Exception: SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: database_tester_authors_roles.author_id, database_tester_authors_roles.role_id

vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDO/Exception.php:18
vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:117
vendor/laravel/framework/src/Illuminate/Database/Connection.php:471
vendor/laravel/framework/src/Illuminate/Database/Connection.php:671
vendor/laravel/framework/src/Illuminate/Database/Connection.php:638
vendor/laravel/framework/src/Illuminate/Database/Connection.php:472
vendor/laravel/framework/src/Illuminate/Database/Connection.php:424
vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2838
rain/src/Database/QueryBuilder.php:304
rain/src/Database/Relations/BelongsToMany.php:136
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:175
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:112
rain/src/Database/Relations/BelongsToMany.php:339
rain/src/Support/Traits/Emitter.php:120
rain/src/Database/Model.php:168
rain/src/Events/Dispatcher.php:397
rain/src/Events/Dispatcher.php:242
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:189
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:898
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:869
rain/src/Database/Model.php:798
rain/src/Database/Model.php:831
tests/unit/plugins/database/BelongsToManyModelTest.php:57

Caused by
PDOException: SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: database_tester_authors_roles.author_id, database_tester_authors_roles.role_id

vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:115
vendor/laravel/framework/src/Illuminate/Database/Connection.php:471
vendor/laravel/framework/src/Illuminate/Database/Connection.php:671
vendor/laravel/framework/src/Illuminate/Database/Connection.php:638
vendor/laravel/framework/src/Illuminate/Database/Connection.php:472
vendor/laravel/framework/src/Illuminate/Database/Connection.php:424
vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2838
rain/src/Database/QueryBuilder.php:304
rain/src/Database/Relations/BelongsToMany.php:136
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:175
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php:112
rain/src/Database/Relations/BelongsToMany.php:339
rain/src/Support/Traits/Emitter.php:120
rain/src/Database/Model.php:168
rain/src/Events/Dispatcher.php:397
rain/src/Events/Dispatcher.php:242
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:189
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:898
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:869
rain/src/Database/Model.php:798
rain/src/Database/Model.php:831
tests/unit/plugins/database/BelongsToManyModelTest.php:57

2) ImageResizerTest::testURLSources
October\Rain\Exception\SystemException: Unable to process the provided image: 'http://localhost/backend/backend/files/get/MSFjMTlhNzcyNzVkNDU5N2E4NWFlYTIxZmFkMTdlNjU0Ng=='

system/Classes/ImageResizer.php:644
system/Classes/ImageResizer.php:98
tests/unit/system/classes/ImageResizerTest.php:261

--

There were 10 failures:

1) CmsExceptionTest::testCmsExceptionPhp
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'tests/fixtures/themes/test/pages/throw-php.htm'
+'/modules/cms/classes/CodeParser.php(165) : eval()'d code line 7'

tests/unit/cms/classes/CmsExceptionTest.php:39

2) AttachOneModelTest::testDeleteFlagDeleteModel
Failed asserting that System\Models\File Object &000000002be9120b000000007dc2cc01 (
    'table' => 'system_files'
    'morphTo' => Array &0 (
        'attachment' => Array &1 ()
    )
    'fillable' => Array &2 (
        0 => 'file_name'
        1 => 'title'
        2 => 'description'
        3 => 'field'
        4 => 'attachment_id'
        5 => 'attachment_type'
        6 => 'is_public'
        7 => 'sort_order'
        8 => 'data'
    )
    'guarded' => Array &3 ()
    'hidden' => Array &4 (
        0 => 'attachment_type'
        1 => 'attachment_id'
        2 => 'is_public'
    )
    'appends' => Array &5 (
        0 => 'path'
        1 => 'extension'
    )
    'data' => null
    'autoMimeTypes' => Array &6 (
        'docx' => 'application/msword'
        'xlsx' => 'application/excel'
        'gif' => 'image/gif'
        'png' => 'image/png'
        'jpg' => 'image/jpeg'
        'jpeg' => 'image/jpeg'
        'webp' => 'image/webp'
        'pdf' => 'application/pdf'
        'svg' => 'image/svg+xml'
    )
    'implement' => null
    'attributes' => Array &7 (
        'id' => '33'
        'disk_name' => '60112c8729bcb281745191.png'
        'file_name' => 'avatar.png'
        'file_size' => '1505'
        'content_type' => 'image/png'
        'title' => null
        'description' => null
        'field' => 'avatar'
        'attachment_id' => '1'
        'attachment_type' => 'Database\Tester\Models\User'
        'is_public' => '1'
        'sort_order' => '33'
        'created_at' => '2021-01-27 09:04:07'
        'updated_at' => '2021-01-27 09:04:07'
    )
    'jsonable' => Array &8 ()
    'dates' => Array &9 ()
    'duplicateCache' => true
    'connection' => 'sqlite'
    'primaryKey' => 'id'
    'keyType' => 'int'
    'incrementing' => true
    'with' => Array &10 ()
    'withCount' => Array &11 ()
    'perPage' => 15
    'exists' => true
    'wasRecentlyCreated' => false
    'original' => Array &12 (
        'id' => '33'
        'disk_name' => '60112c8729bcb281745191.png'
        'file_name' => 'avatar.png'
        'file_size' => '1505'
        'content_type' => 'image/png'
        'title' => null
        'description' => null
        'field' => 'avatar'
        'attachment_id' => '1'
        'attachment_type' => 'Database\Tester\Models\User'
        'is_public' => '1'
        'sort_order' => '33'
        'created_at' => '2021-01-27 09:04:07'
        'updated_at' => '2021-01-27 09:04:07'
    )
    'changes' => Array &13 ()
    'casts' => Array &14 ()
    'classCastCache' => Array &15 ()
    'dateFormat' => null
    'dispatchesEvents' => Array &16 ()
    'observables' => Array &17 ()
    'relations' => Array &18 ()
    'touches' => Array &19 ()
    'timestamps' => true
    'visible' => Array &20 ()
    'hasMany' => Array &21 ()
    'hasOne' => Array &22 ()
    'belongsTo' => Array &23 ()
    'belongsToMany' => Array &24 ()
    'morphOne' => Array &25 ()
    'morphMany' => Array &26 ()
    'morphToMany' => Array &27 ()
    'morphedByMany' => Array &28 ()
    'attachOne' => Array &29 ()
    'attachMany' => Array &30 ()
    'hasManyThrough' => Array &31 ()
    'hasOneThrough' => Array &32 ()
    'emitterSingleEventCollection' => Array &33 ()
    'emitterEventCollection' => Array &34 ()
    'emitterEventSorted' => Array &35 ()
    'extensionData' => Array &36 (
        'extensions' => Array &37 ()
        'methods' => Array &38 ()
        'dynamicMethods' => Array &39 ()
        'dynamicProperties' => Array &40 ()
    )
    'sessionKey' => null
) is null.

tests/unit/plugins/database/AttachOneModelTest.php:85

3) NestedTreeModelTest::testListsNested
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     7 => 'Category Green'
     8 => '   Winter Snow'
     9 => '   Spring Trees'
+    10 => 'Category Orange'
+    11 => '   Autumn Leaves'
+    12 => '    &nbsp...tember'
+    13 => '    &nbsp...ctober'
+    14 => '    &nbsp...vember'
+    15 => '   Summer Breeze'
+    16 => 'Category Green'
+    17 => '   Winter Snow'
+    18 => '   Spring Trees'
 )

tests/unit/plugins/database/NestedTreeModelTest.php:46

4) RevisionableModelTest::testUpdateNothing
Failed asserting that 1 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:21

5) RevisionableModelTest::testUpdateSingleField
Failed asserting that 1 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:33

6) RevisionableModelTest::testUpdateMultipleFields
Failed asserting that 5 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:57

7) RevisionableModelTest::testExceedingRevisionLimit
Failed asserting that 1 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:88

8) RevisionableModelTest::testSoftDeletes
Failed asserting that 5 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:112

9) RevisionableModelTest::testRevisionDateCast
Failed asserting that 1 matches expected 0.

tests/unit/plugins/database/RevisionableModelTest.php:128

10) MediaLibraryTest::testListAllDirectories
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => '/'
-    1 => '/dir'
-    2 => '/dir/sub'
-    3 => '/hidden but not really'
-    4 => '/name'
+    1 => '/.ignore1'
+    2 => '/.ignore2'
+    3 => '/dir'
+    4 => '/dir/sub'
+    5 => '/hidden'
+    6 => '/hidden/sub1'
+    7 => '/hidden/sub1/deep1'
+    8 => '/hidden/sub2'
+    9 => '/hidden but not really'
+    10 => '/name'
 )

tests/unit/system/classes/MediaLibraryTest.php:123

@bennothommo
Copy link
Contributor

@filipac Are you able to produce a PR with your changes so far? Might be worth it so that we can see it when run against the unit tests in our CI config, and I can try and reproduce these test results locally.

@daftspunk daftspunk self-assigned this Mar 8, 2021
@daftspunk
Copy link
Member

I made the upgrade changes today, there is a little more to it than what is described. It's good to know these things early so we can prepare for what's coming. Everything is ready to go in a branch for when the time comes but we won't release with it just yet because it will be too disruptive in addition to the wider release that is coming shortly

Thanks @filipac ! 💪

@vanthao03596
Copy link

Sorry for comment in closed issue. Do we have plan to upgrade to laravel 8 ?. I want to use batch job but laravel 6 doesn't support

@SebastiaanKloos
Copy link
Contributor

Sorry for comment in closed issue. Do we have plan to upgrade to laravel 8 ?. I want to use batch job but laravel 6 doesn't support

It's already mapped on the roadmap for Laravel 9. It's v2 only.

https://portal.octobercms.com/c/27-laravel-9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants