[#9220] - Wrong column name used by schema manager for mysql
- Closed
- 26 Feb 2016
- Medium
- Build: staging
- # 9220
- Diff
- richard67:patch-1
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Summary of Changes
For "ALTER TABLE tablename CHANGE columnname_old columname_new ..." statements in mysql, the database schema manager builds the check query for existence of the column with the old and not the new column name.
Testing Instructions
Either by code review, compare with the preceeding check of "ALTER TABLE tablename MODIFY columnname" statement and check which array elements are used for building the check query and the message,
or
Add statement "ALTER TABLE #__menu CHANGE gaga_old gaga_new VARCHAR(100)" to the latest update sql script for mysqli, go in backend to "Manage -> Database" and check which column name is reported to be missing.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
wilsonge
- | Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-02-26 13:07:04 |
| Closed_By | ⇒ | wilsonge |
| Milestone |
Added: |
||
Please revert this. It is wrong, proof is simple:
Both MODIFY and CHANGE statements are of type CHANGE_COLUMN_TYPE. This assumes the column name doesn't change. So, there is no need to change it!
Changing it to [5] suggests [4] and [5] can be different, which is not the case! Consider to suggest to rename an element is allowed is wrong.
Then it was wrong before also, because no check was made that old and new name are not different. But if a name has to be inclided into the check query, then it is the new name and not the old one. So this merge here was correct.
You are free to make a PR to extend the schema manager by a test if old and new name are the same and if not let it raise an exception because of invalid SQL or whatever.
But ACCORDING TO SQL SYNTAX THIS COMMIT HERE IS CORRECT. PEROD.
I'm not following you here. Specifically, this:
Consider to suggest to rename an element is allowed is wrong
It's not this API's responsibility to block renaming columns, if I'm reading what you said right (and if I didn't everything after this point is invalid). Maybe it's not an action we want in the core CMS, but this library can be used by extensions too and as such shouldn't dictate that extensions cannot rename columns. It also puts the core in a bind if columns do get renamed for whatever reason at 4.0 or later.
And do you agree with me?
You may have noticed that I mentioned about the poor technical solutions of Extensions->Database(->Fix). Having only one query to verify whether something needs to done is one of them.
Obviously, I have nothing against implementing support for renaming a column. It is just not there yet.
Agree that it is not the API responsibility to block it, too bad the API is so poorly documented. Considering the poor quality I am not surprised. With all these limitations documented properly it would probably not have made it into the product.
For support the renaming of a column you can change [4] to [5] as suggested. Obviously that will skip the test whether [4] is not there. Probably acceptable for all those that can live with this poor quality.
So, I rest my case. It seems this project rather spends time on fixing bad code (making it worse) instead of writing the right code!
Well, the check query could be extended by a union all for the old column, so if 2 records would be returned the check would fail, also if no record, but this would be silly because the column change is one statement, and if this failed the new column name will not exist, and so a check for the new name only is absolutely sufficient.
For validating changesets are applied, this aspect of the API needs to keep improving to correctly validate schema changes are actually applied. A new API in full needs to also be written which can reverse engineer an expected schema (joomla.sql or any extension's install SQL file, for those still using core standards) and validate the full schema is in the expected state and the Extension Manager then rely on this newer API as it should in theory be the most accurate point of comparison versus the existing changeset API. The changeset API is OK for a quick reference to check schema version, but it shouldn't be relied on to validate the full database schema is in an expected state.
Personally, I'm not interested in investing time in any of it. Frankly, I know not to use Joomla's database layer or the CMS' schema API for anything regarding schema validation and I'll just roll my own tools on a per-project basis or run manual tools to keep up with everything if I need it. Instead of working on Joomlaravel 4.0 these are the types of issues that need the attention of J4WG.
@brianteeman Please show me some respect!
It is clear there is something wrong with your understanding. Both @mbabker and @nikosdion are saying the same thing.
But don't worry, this was my last contribution.
@wilsonge New PR as discussed in my other one, please check.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9220.