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

Fix EZP-21585: set correct col. length for oracle #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gggeek
Copy link
Contributor

@gggeek gggeek commented Oct 25, 2013

I detail: properly declare all oracle columns lengths using CHAR semantics, to allow for UTF8.
While at it, set the length for all BLOBS we shorten to 4k instead of arbitrary 3000 or 3100

Meant to replace both #18 and ezsystems/ezpublish-legacy#811

@gggeek
Copy link
Contributor Author

gggeek commented Oct 25, 2013

Note: now when we use ColumnTypeTranslation in dbschema.ini.append.php, the max length for varchar cols has to be set to 1000.
Affected known extensions: ezsurvey.
Maybe a BC note would also be good.

@bdunogier
Copy link
Member

In ref to the other issues with the "hack".

To be clear, @gggeek, your fix is clearly better. A bit of a shame it came
after the merge, but still better.

What bothers me a bit is that ezoracle is no longer actively developped. It
is supposed to be maintained, that's all.

And while it is better from a technical pov, I'm not sure of what to think
from a support pespective. We could have it in master, of course, but it
wouldn't do our customers any good.

@gggeek
Copy link
Contributor Author

gggeek commented Oct 25, 2013

Indeed it poses a bit of a challenge:

  • more change for existing customers (risk)
  • but less chances of extra bugs in the future from new customers (f.e. when they could complain that in russian or japanese they can not really go all the way up to 255 chars for the title) (benefit)

@dpobel
Copy link
Contributor

dpobel commented Oct 28, 2013

The update SQL script is missing otherwise the upgrade check won't like it.

@gggeek
Copy link
Contributor Author

gggeek commented Oct 28, 2013

Well, my idea was that customers could actually use the upgrade-check exactly to get the sql generated for them which would set the schema to the new version.
But if this workflow is not considered acceptable, I can provide a script, of course (NB: it is going to be a php script, not an SQL one, as it has to iterate over all tables, including the ones coming from extensions)

@dpobel
Copy link
Contributor

dpobel commented Oct 28, 2013

you can not just send the customers to the upgrade check to get the SQL, if for instance they have a custom table, it will suggest to remove it.
The usual practice is to have either a SQL or php script to update the database schema (nothing new here...). IMHO, the SQL would be better, the tables to change are known.

@gggeek
Copy link
Contributor Author

gggeek commented Oct 28, 2013

Nitpick: suggestion for custom tables to be removed only happens if extensions do not implement the proper .dba table definition as per the standard. I know that a lot of user-extensions (and sadly even some eZ-ones) do not respect that standard, so I agree that we can not just uphold it blindly.

As for table list: no it is not known, as this change applies to all tables, including ones from extensions - the only way to do the change in pure sql would be via a PL-SQL program

@dpobel
Copy link
Contributor

dpobel commented Oct 28, 2013

The tables from our extensions that need to be fixed should be identified and the corresponding setting should then be added to dbschema.ini.append.php otherwise the upgrade check will report a warning.

… semantics, to allow for UTF8. While at it, set the length for all BLOBS we shorten to 4k instead of arbitrary 3000 or 3100
@gggeek
Copy link
Contributor Author

gggeek commented Oct 28, 2013

Added update script - as PLSQL procedure.

As for other extensions, I used an advanced search on github (https://github.com/search?q=ColumnTypeTranslation+%40ezsystems+extension%3Aphp&type=Code&ref=advsearch&l=PHP)
and found only ezsurvey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants