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

Hardcoded makeLegacyMessageCode() limit #26

Open
matteotrubini opened this issue Mar 10, 2022 · 2 comments
Open

Hardcoded makeLegacyMessageCode() limit #26

matteotrubini opened this issue Mar 10, 2022 · 2 comments

Comments

@matteotrubini
Copy link
Contributor

return Str::limit(trim($messageId, $separator), 250);

is ported from
return Str::limit(trim($messageId, $separator), 250);

but it will trown an error in case of MySQL default config with varcharmax = 191 (see https://github.com/wintercms/winter/blob/ec46549b21893d39c15261aa237e49c9c0680fbb/config/database.php#L70)

I think we need to get limit value using the same logic used here:
https://github.com/wintercms/winter/blob/ec46549b21893d39c15261aa237e49c9c0680fbb/modules/system/ServiceProvider.php#L624-L642
of even better, we can check the current table schema with Schema::getConnection()->getDoctrineColumn(self::TABLE_NAME, 'code')->getLength(), but this should be tested against all supported databases to check returned values (I don't have this kind of expertise).

@LukeTowers
Copy link
Member

@matteotrubini it might be worth pointing out that the varcharmax configuration item is removed in Winter v1.2 since it was a workaround for older version of MySQL / MariaDB that are no longer supported in Winter v1.2 so this may become a non-issue.

@matteotrubini
Copy link
Contributor Author

You are right, but what's happens if someone install on / upgrades an existing DB? As varchar columns were created with that lenght, the limit still apply.

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

No branches or pull requests

2 participants