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

[postgresql] - schema change rework #18483

Closed
wants to merge 8 commits into from
Closed

[postgresql] - schema change rework #18483

wants to merge 8 commits into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Nov 2, 2017

Pull Request for Issue #14331 and porting of #17351.

Summary of Changes

support these schema changes:

  • ALTER TABLE "#__table" ALTER COLUMN "field" SET NOT NULL;
  • ALTER TABLE "#__table" ALTER COLUMN "field" SET DEFAULT value;
  • ALTER TABLE "#__table" ALTER COLUMN "field" DROP DEFAULT;
  • ALTER TABLE "#__table" ALTER COLUMN "field" DROP NOT NULL;

Testing Instructions

With latest staging on postgresql go to Manage > Database.

Expected result

no notice

Actual result

Notice: Undefined offset:....

@waader , @csthomas please re-test, review

@csthomas
Copy link
Contributor

csthomas commented Nov 5, 2017

  1. I have not spent enough time on it but when I changed database table by:
ALTER TABLE "#__extensions" ALTER COLUMN "custom_data" DROP NOT NULL;
ALTER TABLE "#__extensions" ALTER COLUMN "custom_data" SET DEFAULT '';

then joomla does not see any problems, but should show that DEFAULT values was removed in

ALTER TABLE "#__extensions" ALTER COLUMN "custom_data" DROP DEFAULT;

  1. There is a white space on line 117.

@mbabker mbabker added this to the Joomla 3.8.4 milestone Dec 11, 2017
return a row only when the column have a default
@alikon
Copy link
Contributor Author

alikon commented Dec 30, 2017

1 and 2 fixed

@alikon alikon changed the title [postgresql] - remove notice from manage database [postgresql] - schema change rework Jan 2, 2018
@alikon
Copy link
Contributor Author

alikon commented Jan 2, 2018

reworked a little bit for use information_schema.columns

. ' ELSE '
. ' substring(column_default, 1, (position('. $this->db->quote('::') . ' in column_default) -1)) = ' . $this->db->quote($string)
. ' END)';
$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 139-141 Remove tab

$result = 'SELECT column_name, data_type, is_nullable , column_default FROM information_schema.columns'
. ' WHERE table_name=' . $this->fixQuote($wordArray[2]) . ' AND column_name=' . $this->fixQuote($wordArray[5])
. ' AND is_nullable = ' . $isNullable;
$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 163-165 Remove tab

}
else
break;
case 'SET' :
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line before

. ' AND data_type=' . $this->fixQuote($type);
$result = 'SELECT column_name, data_type FROM information_schema.columns WHERE table_name='
. $this->fixQuote($wordArray[2]) . ' AND column_name=' . $this->fixQuote($wordArray[5])
. ' AND data_type=' . $this->fixQuote($type);

$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 112-166 indent

}
else
break;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove tabs

@alikon
Copy link
Contributor Author

alikon commented Jan 3, 2018

@Quy hope cs better now

$this->queryType = 'CHANGE_COLUMN_TYPE';
$this->msgElements = array($this->fixQuote($wordArray[2]), $this->fixQuote($wordArray[5]), $type);
break;
case 'SET' :
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line before line 115.

$this->checkQueryExpected = 0;
$this->msgElements = array($this->fixQuote($wordArray[2]), $this->fixQuote($wordArray[5]), 'NOT NULL');
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is missing closing } to the switch statement.

. ' ELSE '
. ' substring(column_default, 1, (position('. $this->db->quote('::') . ' in column_default) -1)) = ' . $this->db->quote($string)
. ' END)';
$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line above.

$result = 'SELECT column_name, data_type, is_nullable , column_default FROM information_schema.columns'
. ' WHERE table_name=' . $this->fixQuote($wordArray[2]) . ' AND column_name=' . $this->fixQuote($wordArray[5])
. ' AND column_default IS NULL';
$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line above.

$result = 'SELECT column_name, data_type, is_nullable , column_default FROM information_schema.columns'
. ' WHERE table_name=' . $this->fixQuote($wordArray[2]) . ' AND column_name=' . $this->fixQuote($wordArray[5])
. ' AND is_nullable = ' . $isNullable;
$this->queryType = 'CHANGE_COLUMN_TYPE';
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line above.

. $this->fixQuote($wordArray[2]) . ' AND column_name=' . $this->fixQuote($wordArray[5])
. ' AND (CASE (position(' . $this->db->quote('::') . ' in column_default))'
. ' WHEN 0 THEN '
. ' column_default = ' . $this->db->quote($string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent lines 136-139.

@alikon
Copy link
Contributor Author

alikon commented Jan 3, 2018

sorry & thanks @Quy for your extra-work still fighting with the local setup of phpcs 🤕

@mbabker mbabker removed this from the Joomla 3.8.4 milestone Jan 30, 2018
@alikon
Copy link
Contributor Author

alikon commented Feb 21, 2018

please test #19707

@alikon alikon closed this Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants