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

Shorten index name of calendar changes table #12447

Merged
merged 1 commit into from Nov 15, 2018

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Nov 14, 2018

Fix #12446
Fix #12410

@nickvergessen
Copy link
Member Author

@nickvergessen nickvergessen commented Nov 14, 2018

@nickvergessen nickvergessen force-pushed the bugfix/12446/fix-oracle-install-again branch from 36365da to 38752aa Nov 14, 2018
@MorrisJobke MorrisJobke mentioned this pull request Nov 14, 2018
24 tasks
@@ -279,7 +279,7 @@ public function isClassMapAuthoritative()
*/
public function setApcuPrefix($apcuPrefix)
{
$this->apcuPrefix = function_exists('apcu_fetch') && filter_var(ini_get('apc.enabled'), FILTER_VALIDATE_BOOLEAN) ? $apcuPrefix : null;
$this->apcuPrefix = function_exists('apcu_fetch') && ini_get('apc.enabled') ? $apcuPrefix : null;
Copy link
Member

@MorrisJobke MorrisJobke Nov 14, 2018

This looks like you are using an outdated composer. Because my local composer (which says is up to date) also produced the output as stated in the deleted line and the checker CI job also complains.

Copy link
Member Author

@nickvergessen nickvergessen Nov 14, 2018

yeah, still had 1.7.2, now all other ClassLoaders where updated to the filter_var version

/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

if ($schema->hasTable('calendarchanges')) {
Copy link
Member

@MorrisJobke MorrisJobke Nov 14, 2018

Why is this needed? Can't we assume, that it is there? Same for the other two if statements below.

Copy link
Member Author

@nickvergessen nickvergessen Nov 14, 2018

I just copied the original migration, yes we could assume it exists, but does it hurt?

Copy link
Member

@MorrisJobke MorrisJobke Nov 14, 2018

Not really - it just confuses.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 14, 2018

I guess it's not wrong to use short table/index/whatever names but oracle 11 is eol (without a subscription) since 2017. The 30 characters limitation has been increased to 128 with 12.2 (https://blog.dbi-services.com/oracle-12cr2-long-identifiers/). Maybe running the tests with a newer oracle db would work? 😕

@nickvergessen
Copy link
Member Author

@nickvergessen nickvergessen commented Nov 14, 2018

Well most people that use Oracle in a larger instance have a subscription.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Tested and works 👍

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Nov 15, 2018

@rullzer Mind to review?

@MorrisJobke MorrisJobke merged commit d70b010 into master Nov 15, 2018
1 check failed
@MorrisJobke MorrisJobke deleted the bugfix/12446/fix-oracle-install-again branch Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants