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

[ticket/12873] Test the correct identifier in \phpbb\db\tools #2761

Merged
merged 3 commits into from Aug 7, 2014

Conversation

Nicofuma
Copy link
Member

@bantu
Copy link
Collaborator

bantu commented Jul 20, 2014

Can you explain why there are changes to the schema file?

@bantu
Copy link
Collaborator

bantu commented Jul 20, 2014

I think instead of "good" you mean "correct". The first one does not really work in that context.

@Nicofuma Nicofuma changed the title [ticket/12873] Test the good identifier in \phpbb\db\tools [ticket/12873] Test the correct identifier in \phpbb\db\tools Jul 20, 2014
@Nicofuma
Copy link
Member Author

For the change in the schema file, we need to rename the index key because with the default prefix (phpbb_) this key has a length of 31. And because we don't accept the keys longer than 30 characters we should ensure that by default no key is longer than 30 characters.
We could use a a migration for that, but it seems a little overkill: this key is already present in Olympus so it will only affect new installations.

@bantu
Copy link
Collaborator

bantu commented Jul 20, 2014

For the change in the schema file, we need to rename the index key because with the default prefix (phpbb_) this key has a length of 31. And because we don't accept the keys longer than 30 characters we should ensure that by default no key is longer than 30 characters.

This information should be provided as part of the commit message.

We could use a a migration for that, but it seems a little overkill: this key is already present in Olympus so it will only affect new installations.

Are you aware that schema.json is an automatically generated file?

@Nicofuma
Copy link
Member Author

.... off course I know... I just forgot to fix the migration instead....

We need to rename the index key because with the default prefix (phpbb_)
this key has a length of 31. And because we don't accept the keys longer
than 30 characters we should ensure that by default no key is longer than
30 characters.

PHPBB3-12873
@Nicofuma
Copy link
Member Author

updated

@@ -731,7 +731,7 @@ public function update_schema()
'title_match' => array('BOOL', 0),
),
'KEYS' => array(
'unq_mtch' => array('UNIQUE', array('word_id', 'post_id', 'title_match')),
'un_mtch' => array('UNIQUE', array('word_id', 'post_id', 'title_match')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Arg, this seems like a bad idea. It will cause 2 different index names depending on whether you install 3.1 or updated from 3.0
However dropping the index and recreating it might be even more expensive. But I guess it's what we should do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't rename indexes?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we also can not rename columns

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be very very expensive on big boards... but it is very mattering if we have 2 different index names?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in this case its better to have two different names. It's unlikely that we aant to drop the index later anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

@Nicofuma we decided to go with a new migration and just make the upgrade take a while.
So drop the old index and create a new one in a new migration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(The db index that is, not the whole search index.)

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@Nicofuma Nicofuma added WIP and removed WIP labels Jul 28, 2014
nickvergessen added a commit to nickvergessen/phpbb that referenced this pull request Aug 7, 2014
[ticket/12873] Test the correct identifier in \phpbb\db\tools

* Nicofuma/ticket/12873:
  [ticket/12873] Add migration to rename the index
  [ticket/12873] Don not touch the existing migrations
  [ticket/12873] Test the good identifier in \phpbb\db\tools
@nickvergessen nickvergessen merged commit 2b3e15c into phpbb:develop-ascraeus Aug 7, 2014
nickvergessen added a commit that referenced this pull request Aug 7, 2014
[ticket/12873] Test the correct identifier in \phpbb\db\tools

* Nicofuma/ticket/12873:
  [ticket/12873] Add migration to rename the index
  [ticket/12873] Don not touch the existing migrations
  [ticket/12873] Test the good identifier in \phpbb\db\tools
@Nicofuma Nicofuma removed the WIP label Aug 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants