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

Add the ce_access database migration #1175

Merged
merged 5 commits into from Jan 13, 2020
Merged

Add the ce_access database migration #1175

merged 5 commits into from Jan 13, 2020

Conversation

@leofeyer
Copy link
Member

leofeyer commented Jan 13, 2020

Fixes #1133

@leofeyer leofeyer added the defect label Jan 13, 2020
@leofeyer leofeyer added this to the 4.9 milestone Jan 13, 2020
@leofeyer leofeyer requested review from ausi, Toflar, bytehead and aschempp Jan 13, 2020
@leofeyer leofeyer self-assigned this Jan 13, 2020
@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Jan 13, 2020

@contao/developers Should we encapsulate the migration services in a separate migrations.yml file?

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 13, 2020

@contao/developers Should we encapsulate the migration services in a separate migrations.yml file?

I'd prefer so

Copy link
Member

Toflar left a comment

It looks good to me in general. Not sure about the naming though so I'd like @ausi to have a word on this too.

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Jan 13, 2020

migrations.yml file added in 21d6a5a.

$this->assertTrue($migration->run()->isSuccessful());
}

public function testDoesNothingIfTheElementsColumnExists(): void

This comment has been minimized.

Copy link
@ausi

ausi Jan 13, 2020

Member

Shouldn’t this be name somthing like this?

Suggested change
public function testDoesNothingIfTheElementsColumnExists(): void
public function testDoesNothingIfTheUserGroupTableDoesNotExist(): void

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jan 13, 2020

Author Member

Fixed in ddf1627.

I am not sure how much sense these tests make, though. (I am talking about the whole class, not just the two methods added/fixed in the most recent commit.)

This comment has been minimized.

Copy link
@ausi

ausi Jan 13, 2020

Member

I’m not sure either. Should we make a functional test instead that migrates a real database?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jan 13, 2020

Author Member

I am not sure. The unit test already ensures that the correct query parameters are used, so a function tests would basically only test that the application can write to the database. 🤷‍♂

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Jan 13, 2020

Not sure about the naming though

I generally like the naming as it is. But we could rename the Version40900 folder to Version409 IMO. If we add a migration in Version 4.9.1 it is OK I think to add it to the same folder.

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Jan 13, 2020

But we could rename the Version40900 folder to Version409 IMO

Renamed in 96fc6dc.

@ausi
ausi approved these changes Jan 13, 2020
@Toflar
Toflar approved these changes Jan 13, 2020
@leofeyer leofeyer merged commit 735df94 into master Jan 13, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.72% (+0.02%) compared to 7448f2b
Details
@leofeyer leofeyer deleted the bugfix/ce-access-migration branch Jan 13, 2020
@leofeyer leofeyer changed the title Add the ce_access migration Add the ce_access database migration Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.