Feature: Add method to check VARCHAR support without length specification

berkut1 commented 2 weeks ago

Q	A
Type	feature

Summary

A method has been added to allow disabling the length check for VARCHAR in the ORM3 for platforms that support it. This is related to this issue https://github.com/doctrine/orm/issues/11502.

If this PR is approved, I plan to add the following check in the ORM at: https://github.com/doctrine/orm/blob/9d4f54b9a476f13479c3845350b12c466873fc42/src/Tools/SchemaTool.php#L463-L465

The check is:

       if(! $this->platform->supportsVarcharWithoutLength()) {
            if (strtolower($columnType) === 'string' && $options['length'] === null) {
                $options['length'] = 255;
            }
        }

I am not sure if this should be added to DBAL3 because, due to a bug in schema definition, it is already supported. And considering that the schema definition method was changed in DBAL4 but not in DBAL3, I assume there are some hidden issues or that it was done to avoid breaking backward compatibility for people who are unknowingly using this bug.

greg0ire commented 2 weeks ago

I don't feel great about this… it makes the abstraction leaky. I feel like we should not set a default ourselves at all, and provide a configuration option so that users could specify a default length if they need to.

berkut1 commented 2 weeks ago

@greg0ire I tried to mimic the existing method, as it exists here, but if this is a bad approach, then it might not be worth it.

Regarding making it possible in ORM to specify a default length for VARCHAR, I currently don't see a way to do this without abstraction leak. The string value 255 is hardcoded.

derrabus commented 2 weeks ago

Can you elaborate your use-case for this kind of VARCHAR type? In which situations would you prefer this definition over an explicitly limited VARCHAR or a TEXT column?

If we would add support for that, we should do so in a portable way, e.g. by introducing a new type. What you see as a VARCHAR without a limit could be an NVARCHAR(max) in SQL Server for instance. A bunch of "supports" methods would be a very weak abstraction for this.

berkut1 commented 2 weeks ago

Thank you for the constructive comments. Now I also think that it's a bad idea. :)

Regarding the use case: For PostgreSQL, there is no point in using VARCHAR(n) because VARCHAR works the same way and occupies as much space in the database as the string itself takes. Because of this, we used VARCHAR throughout the database, and when we decided to switch to Doctrine, we did not encounter any problems. It was logical for us that when length = null, Doctrine treats the string as unlimited and accepts VARCHAR. We only found out that this was a bug when we were preparing to switch to ORM3 and DBAL4.

I am now most inclined to think that it would be better to completely remove the check from ORM3 and force everyone who did not specify a length to explicitly specify it when updating ORM3 to avoid such situations. But such a decision would definitely not be approved, so I didn't even try.

In any case, thank you. We can get out of our situation in two ways: switch to the TEXT type or create a custom type for VARCHAR.

doctrine / dbal

Feature: Add method to check VARCHAR support without length specification #6455

Summary