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

Also migrate the "useFolderUrl" setting #1987

Merged
merged 6 commits into from Jul 26, 2020

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Jul 24, 2020

Q A
Fixed issues -
Docs PR or issue -

When upgrading a Contao 4.9 instance to 4.10 I noticed that the folderUrl setting wasn't migrated to the tl_page settings.

Note: I need some help with the unit tests, as I am not sure how to properly mock the Config singleton, which also has static methods.

@fritzmg fritzmg added the bug label Jul 24, 2020
@fritzmg fritzmg self-assigned this Jul 24, 2020
@m-vo
Copy link
Member

m-vo commented Jul 24, 2020

Note: I need some help with the unit tests, as I am not sure how to properly mock the Config singleton, which also has static methods.

What do you need exactly? Maybe you can solve it with $this->mockClassWithProperties(Config::class, [...]);

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 24, 2020

What do you need exactly? Maybe you can solve it with $this->mockClassWithProperties(Config::class, [...]);

Well the static function is not a property. I want to do

$config = $this->createMock(Config::class);
$config
    ->expects($this->once())
    ->method('has')
    ->with('folderUrl')
    ->willReturn(false)
;

But this will fail:

1) Contao\CoreBundle\Tests\Migration\Version410\FolderUrlMigrationTest::testDoesNothingIfFolderUrlNotEnabled
Static method "has" cannot be invoked on mock object

Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

I don't think this will work. If folderUrl is disable in the root pages, the migration will always run again. The correct migration would be to check if the field exists in the database, and only migrate if it does not. That obviously won't fix current RCs, but we don't provide such fixes for RCs.

@leofeyer leofeyer added this to the 4.10 milestone Jul 24, 2020
@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 24, 2020

I don't think this will work. If folderUrl is disable in the root pages, the migration will always run again.

Only if you re-add the setting in your localconfig.php or don't remove contao.localconfig.folderUrl: true from your config.yml.

But generally I agree, the migration should only run, if the database field does not exist yet.

@aschempp
Copy link
Member

It could even be added to the RoutingMigration in my view 😊

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 25, 2020

Alright, I have integrated it into the RoutingMigration now. Which caused me to notice, that the RoutingMigration wasn't actually registered as a service and thus would have never been executed in the first place 🙈

(also, there are no tests what so ever for any 410 migration 😶)

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 26, 2020

Right, forgot about the functional tests. Updated.

@leofeyer leofeyer merged commit 7585b8d into contao:4.10 Jul 26, 2020
@leofeyer
Copy link
Member

Thank you @fritzmg.

@leofeyer leofeyer changed the title Provide migration for folderUrl setting Also migrate the "useFolderUrl" setting Aug 6, 2020
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | -
| Docs PR or issue | -

When upgrading a Contao 4.9 instance to 4.10 I noticed that the `folderUrl` setting wasn't migrated to the `tl_page` settings.

_Note:_ I need some help with the unit tests, as I am not sure how to properly mock the `Config` singleton, which also has static methods.

Commits
-------

5638df5 provide migration for folderUrl setting
69c840f fix code style and namespace
fb2566f use existing RoutingMigration
4e3f302 Merge branch '4.10' into migrate-folderUrl-config
4c9ce4e update functional test
c873d39 fix code style
@fritzmg fritzmg deleted the migrate-folderUrl-config branch October 24, 2021 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants