Add enum phpType support and enum migrations for PostgreSQL

[oojacoboo]

Summary

• Adds BackedEnum detection for phpType columns: hydration generates EnumClass::from($value) instead of new EnumClass($value), and PDO binding extracts ->value for scalar storage
• Enables PostgreSQL native enum support (ENUM_NATIVE) by passing true to setSetTypesMapping() in PgsqlPlatform and overriding buildNativeEnumeratedColumnSqlType() to return VARCHAR (PG enums are named types created via CREATE TYPE, not inline ENUM() syntax — PDO handles them as strings)

Changes

Backed enum phpType support (PropelTypes, Column, ObjectBuilder, ColumnCodeProducer):

• PropelTypes::isPhpBackedEnumType() / Column::isPhpBackedEnumType() — detects PHP backed enums via is_subclass_of(BackedEnum::class)
• Hydration: ClassName::from($columnValue) instead of new ClassName($columnValue)
• PDO binding: $this->column?->value extracts the scalar backing value
• Default values: ClassName::from($default) in applyDefaultValues() and hasOnlyDefaultValues()

PostgreSQL ENUM_NATIVE (PgsqlPlatform):

• setSetTypesMapping(true) — enables native enum type support
• buildNativeEnumeratedColumnSqlType() returns VARCHAR since PG enum columns are read/written as strings via PDO

Usage

<column name="role_type" type="ENUM_NATIVE" required="false" phpType="\App\Model\RoleType" />

Where RoleType is a PHP backed string enum:

enum RoleType: string {
    case Admin = 'admin';
    case Manager = 'manager';
}

Generated code:

// Property
protected RoleType|null $role_type = null;

// Hydration
$this->role_type = ($columnValue === null) ? null : RoleType::from($columnValue);

// PDO binding
$stmt->bindValue($identifier, $this->role_type?->value, PDO::PARAM_STR);

[mringler]

Interesting!

Adjusting phpType behavior for enums is nice! But why only BackedEnum? What about UnitEnum?

Not sure about the Postgres change though. Can you explain what it effectively adds? Does it more than add an alias column type, where you can write ENUM_NATIVE, but it actually is a string column for all intent and purposes? Is it even true, when the database type is not ENUM, but a custom type (that happens to be generated from an enum)?
For a Postgres type RoleType, is

<column name="role_type" type="ENUM_NATIVE" sqlType="RoleType" />

actaully more accurate than just

<column name="role_type" sqlType="RoleType" />

? Particularly when Perpl does not know anything about the type, except it's name?

The advantage of using ENUM_NATIVE for MySQL enum columns is that Perpl generates correct migration code. I think for Postgres, that would mean updating the custom type. Can Perpl do that?
So many questions...

And I think we would need actual runtime tests for these changes (The perpl-test-docker image has Postgres
configuration). Maybe it would make sense to split this up into the phpType part and Postgres part?
What do you think?

[oojacoboo]

So, I think you'll need to review how Postgres handles enums. That'll provide you with color here. You can also review how Doctrine defines enums, and how that works with Postgres.

UnitEnum, could, in theory, be supported as well. But it's kind of strange. I'd argue that it's better to enforce BackedEnums, as it's more explicit - the backed value being the database value. I believe Doctrine requires BackEnum. But I'm happy to add UnitEnum.

<column name="role_type" type="ENUM_NATIVE" phpType="App\Model\RoleType" />

This tells the generator that it's an enum (explicit intent) and that we need to use Enum::from() instead of trying to instantiate an object using the new keyword.

The advantage of using ENUM_NATIVE for MySQL enum columns is that Perpl generates correct migration code. I think for Postgres, that would mean updating the custom type. Can Perpl do that?

Of course it can. But, I don't even know if the MySQL generator works for that? Where is the schema attribute to define all the enum named constants? How else is it going to generate the enum? Also, here is a bit from Doctrine on MySQL enum's shortcomings:

But first a word of warning. The MySQL Enum type has considerable downsides:

• Adding new values requires to rebuild the whole table, which can take hours depending on the size.
• Enums are ordered in the way the values are specified, not in their "natural" order.
• Enums validation mechanism for allowed values is not necessarily good, specifying invalid values leads to an empty enum for the default MySQL error settings. You can easily replicate the "allow only some values" requirement in your Doctrine entities.

Yea, we can split off the Postgres type creation bit. That does need some work. We don't use Propel for migrations. I wonder if anyone actually does.

--

As an aside here, I don't even think type="(ENUM_NATIVE | ENUM_BINARY)" are great ideas. If you enforce BackedEnum and stick with a type that aligns with the storage mechanism, you'll get better results. Doctrine, for instance, does this with a type that's DBMS agnostic (string instead of varchar, for instance). That said, with a BackedEnum, it can return a string or int as it's backed value. This could give you your "native" or "binary" result.

I'm not trying to complicate things here, but we're really trying to hang onto too much BC, for the sake of people that aren't even using enums presently.

Couldn't the type just be "enum"?

The custom attribute that's needed for the migrations would define what else is generated there. Doctrine does something like:

columnDefinition="ENUM('visible', 'invisible')"

[mringler]

Yes, please add UnitEnum.

Beyond that, I'm afraid I don't understand your answer. Why do you want to write

<column name="role_type" type="ENUM_NATIVE" sqlType="RoleType" valueSet="Foo,Bar" />

in Postgres?

[oojacoboo]

Beyond that, I'm afraid I don't understand your answer. Why do you want to write

<column name="role_type" type="ENUM_NATIVE" sqlType="RoleType" valueSet="Foo,Bar" />

in Postgres?

So, this is required for the migration generation to work for Postgres, since the ENUM_NATIVE, will by default, try to use the ENUM() syntax. That syntax is not valid in Postgres.

If someone uses ENUM_NATIVE with phpType but no sqlType, they're managing their schema manually. The SQL type doesn't affect runtime hydration (PDO returns strings regardless, phpType handles PHP conversion). The VARCHAR default for PgsqlPlatform::buildNativeEnumeratedColumnSqlType is just a safe no-op — the DDL won't be executed anyway.

On adding the UnitEnum support. I think a decision on what I mentioned above should be considered first.

As an aside here, I don't even think type="(ENUM_NATIVE | ENUM_BINARY)" are great ideas. If you enforce BackedEnum and stick with a type that aligns with the storage mechanism, you'll get better results. Doctrine, for instance, does this with a type that's DBMS agnostic (string instead of varchar, for instance). That said, with a BackedEnum, it can return a string or int as it's backed value. This could give you your "native" or "binary" result.

I'm not trying to complicate things here, but we're really trying to hang onto too much BC, for the sake of people that aren't even using enums presently.

Couldn't the type just be "enum"?

[oojacoboo]

Okay, I've added support for UnitEnum instances defined as a phpType. I've also gone ahead and implemented enum type migrations for Postgres.

This leaves 2 remaining items for enums before I'd consider it feature complete (not part of this PR):

• Generating base enum classes when defining type="ENUM_NATIVE" but not including a phpType.
• In doing the above, which should be the default for "NATIVE" anything - the need for ENUM_NATIVE and ENUM_BINARY doesn't really make sense to me. Why can't we just use a BackedEnum and back it with a string or int?

[mringler]

Ok, let's recap:

So, I think you'll need to review how Postgres handles enums. That'll provide you with color here. You can also review how Doctrine defines enums, and how that works with Postgres.

?

I think there is a misunderstanding here.
To verify that your change does what you think it does, I need to know what you think it does. From your description and your initial code, it is not clear to me what you are trying to achieve with the changes to PgsqlPlatform. It now seems to become about migrations?
Plz explainz!

On adding the UnitEnum support. I think a decision on what I mentioned above should be considered first

Ok, why should we restrict this? It is technically possible to use UnitEnum, just the same as BackedEnum. Yes, using BackedEnum is probably the better choice. But it might not be a decision the person writing the code can make. We should support both.
Your explanation seems to be about preference, not about impassable limitation. From a technical perspective, that does not seem like a good reason to me. Feel free to clarify.

We don't use Propel for migrations. I wonder if anyone actually does.

Assume yes. It is an important part of Propel/Perpl.

And if this is not about migrations, I don't see what you get from declaring a column as ENUM when it is just a string column on Perpl side. Is it about the consts in TableMap, or what?

we're really trying to hang onto too much BC

If people decide to make the jump from Propel2 to Perpl, I would prefer not to break their system when possible. Weighing necessity against possible negative impact, this does not seem to warrant breaking BC. It is not a front and center issue, and behavior can easily and comfortably be altered through config params:

Couldn't the type just be "enum"?

It is, when you set the config parameter generator.defaultToNativeEnumeratedColumnType, as discussed in #99.

But feel free to provide a different evaluation than gain vs risk. BC is a slippery slope, I am happy to discuss clear parameters for when we should break BC and how to make these changes as painless as possible for users.

Don't get me wrong, I do hope and look forward to a Perpl 3, where we make the changes to the interface permanent.

---

I started a review, but I have to say, I am not comfortable doing these two unrelated things in the same PR. This is going to be confusing when ENUM means two different things in the same PR. Also, it is hard to say at the moment how big either of fixing phpType as PHP Enum and supporting custom types in Postgres is going to get.
Can you please split them up?

[oojacoboo]

Ok, let's recap:

So, I think you'll need to review how Postgres handles enums. That'll provide you with color here. You can also review how Doctrine defines enums, and how that works with Postgres.

?

Based on some of your comments, I assumed there were some misunderstandings for how Postgres handles enums, and it's important that we're speaking the same language here. That's all I meant here. If I misunderstood and you have a thorough understanding for how they work in Postgres, ignore this.

I think there is a misunderstanding here. To verify that your change does what you think it does, I need to know what you think it does. From your description and your initial code, it is not clear to me what you are trying to achieve with the changes to PgsqlPlatform. It now seems to become about migrations? Plz explainz!

Correct, I went ahead and added support for migrations, not for myself, because I couldn't care less, but for the sake of feature completeness.

On adding the UnitEnum support. I think a decision on what I mentioned above should be considered first

Ok, why should we restrict this? It is technically possible to use UnitEnum, just the same as BackedEnum. Yes, using BackedEnum is probably the better choice. But it might not be a decision the person writing the code can make. We should support both. Your explanation seems to be about preference, not about impassable limitation. From a technical perspective, that does not seem like a good reason to me. Feel free to clarify.

UnitEnum support has been added for phpType.

This comment is about something else entirely. Databases that store integers for enums, and are translated into string value constants through Propel are essentially BackedEnums, with a lot of hacky stuff, presently, to accomplish the same objective. BackedEnums can also accomplish string based enums at the database level or integers. So, when generating an enum for a column, we'd want to use BackedEnum for both - not UnitEnum.

To be clear - this is future discussion for generating enums in the Propel model and not implemented in this PR. Perpl does not generate PHP enums yet. But when it does, it should use BackedEnums, which resolve this integer (binary) vs string bifurcation.

We don't use Propel for migrations. I wonder if anyone actually does.

Assume yes. It is an important part of Propel/Perpl.

Right, which is why I went ahead and implemented the migration logic for enums. So, now MySQL and Postgres are properly supported for enum columns (not just MySQL).

And if this is not about migrations, I don't see what you get from declaring a column as ENUM when it is just a string column on Perpl side. Is it about the consts in TableMap, or what?

Perpl needs to know that you're trying to "hydrate" an enum and not an object, when declaring phpType. I think this is the most explicit intent of the schema declaration. Could we leave the type="varchar" for the column and use an emum phpType - sure. But that's not very explicit, and feels pretty hacky, IMO. In Postgres, enums are not "varchar" columns, they're explicitly assigned to the type. Types are created at the schema level and reused (better than MySQL). That's why the syntax isn't ENUM() for every column in the DDL. Instead, you create the type, then you assign it to the columns that use it.

we're really trying to hang onto too much BC

If people decide to make the jump from Propel2 to Perpl, I would prefer not to break their system when possible. Weighing necessity against possible negative impact, this does not seem to warrant breaking BC. It is not a front and center issue, and behavior can easily and comfortably be altered through config params:

I completely agree. But enums were never supported. So, when we're adding ENUM_NATIVE or whatever const we're using for this type, we don't need to support legacy ways of dealing with enums - we shouldn't. Just by introducing them, we're creating debt. My point here is... ENUM_NATIVE and ENUM_BINARY was probably a poor decision, in retrospect. What we should have done, and I didn't really realize at the time, is properly generate enums in the model with Perpl for columns defined as ENUM_NATIVE. That covers the string/int bifurcation issue and cleans up the API, at the same time. The getters and setters implement the enum type directly, not the string/int. That's a change. But it's also a new const that you're opting into. It's also the expected result of this type/const IMO.

Couldn't the type just be "enum"?

It is, when you set the config parameter generator.defaultToNativeEnumeratedColumnType, as discussed in #99.

::thumbs_up::

But feel free to provide a different evaluation than gain vs risk. BC is a slippery slope, I am happy to discuss clear parameters for when we should break BC and how to make these changes as painless as possible for users.

Don't get me wrong, I do hope and look forward to a Perpl 3, where we make the changes to the interface permanent.

See my comments above. We can continue this bit on #97 as well and focus on what's in this PR.

I started a review, but I have to say, I am not comfortable doing these two unrelated things in the same PR. This is going to be confusing when ENUM means two different things in the same PR. Also, it is hard to say at the moment how big either of fixing phpType as PHP Enum and supporting custom types in Postgres is going to get. Can you please split them up?

I can try to split them. But they're not unrelated. They're very tied together. For Postgres, you cannot define ENUM_NATIVE as the column type, to let you know how to initialize the phpType with the way the code is currently. It's made assumptions that MySQL is the only RDBMS that supports native enums, and this is simply incorrect. So, it's really a bug, b/c ENUM_NATIVE breaks Postgres support. You cannot generate, as ENUM() is not valid syntax. That's not because of some limitation of Postgres. It's because MySQL and Postgres handle enums differently.

Now, to split them. I can add a hack as mentioned above about the type="varchar" phpType="\App\Enum", but as I stated, I don't think this is the correct decision (it's implicit intent), especially when this PR prevents the need for that and implements it all uniformly.

[mringler]

From my understanding, you have to be careful not to mix up DB type with PHP type. You are mapping between them - they are not the same, and they are not as tightly coupled as (it sounds like) you say they are.

CHAR or VARCHAR has a specific meaning about how "char sequences" are stored in DB (not sure if it is still implemented that way, but CHAR used to mean that the value is stored "inline" as part of the tuple, while VARCHAR stores a pointer in the tuple which points to the actual value). Saying that's the same as string in PHP would simply be incorrect.

The same is true for enums imo. As you said yourself, PHP enum data can be stored in DB as CHAR, VARCHAR, INT, or, if available, as ENUM. The DB does not need to know that a column will be restored as PHP enum, and PHP does not need to know if it is reading a VARCHAR or an ENUM column.

A tight coupling between the two might be desirable for a developer, but it is easy to construct a scenario, where it is not:

• When you use Sqlite, you might want to use PHP enums, but you have to save them as text (or int with the ENUM_BINARY), because there is no ENUM column type in Sqlite.
• When your system is a simple REST service, that just writes what it gets to storage and returns it again, going from string to PHP enum back to string is just overhead, and you might prefer to leave validation to the database through CHECK and/or ENUM. (I'm not saying this is a good approach, but who am I to judge others constraints?)
• Maybe your boss hates PHP enums, but loves DB enums (or vice versa).

Perpl has three ways to give "the" type of a field:

• phpType, which specifies the type to use on PHP side
• sqlType, which specifies the column type in DB
• type, which defines what code should be produced in model/query/tablemap classes and migrations. It implicates phpType and sqlType, but they can be overridden.

type is obviously the most consequential attribute, but the three are not as clearly separated as one might hope (you can easily create dysfunctional configurations; sqlType might require a change in PHP code; etc).

I think you argue that a type of ENUM(_NATIVE) should use a DB column type of ENUM and generate PHP code that uses PHP enum?
But should it though?

• What happens if you say type="ENUM_NATIVE", but do not specify the actual PHP enum in phpType?
• What do you do when you want to use the column type ENUM, but just use a string on PHP side?

To me, it seems obvious that the default phpType has to be string, as it is the only one that can safely be used in all circumstances. Giving a PHP enum in phpType changes the generated code, but just the same as with any other schema type.
At the same time, using type="ENUM(_NATIVE)" by itself does not need to generate different model/query/tablemap code than a string column, but it impacts what migration code should be produced. For example, changing the valueSet should create migrations that update the int values in the column accordingly (Propel/Perpl does not do that atm, and I think it's a huge problem with the binary type).

With all that, I hope it becomes clear why I think you are doing three things in this PR:

• make phpType work with PHP enums
• make type="ENUM(_NATIVE)" create usable migrations on Postgres when combined with sqlType
• work toward tightly coupling PHP enum and SQL ENUM

The first one I applaud, it fixes a bug in the system.
The second one I find interesting, though not without pitfalls (as I have mentioned before, Postgres does not actually allow you to use ENUM, it allows you to use a custom type that might - or might not - be based on ENUM; it is important to understand that there is a difference between the two). I think it became clear that this is more than just flipping a switch in PgsqlPlatform and be done with it.
The third one I think is not feasible, since the way DB type and PHP type are connected is more complex than that and it makes the configuration in schema less expressive (as I hope became clear in the above)

Does that make sense, is it understandable?

One way or another, creating the Postgres migration code affects different classes than making phpType work. The two are actually not related at all, once you discard an "implicit" coupling of the two in the mind of a developer.
Making phpType work with PHP enums seems straight forward and compact.
Chaning Postgres migrations seems non-trivial, it does not strike me as something that should be done passing by, and I find it hard to predict how much work it actually takes to implement this properly, particularly when including tests.
As a reviewer, it is much more complicated when you always have to discern which of the two goals a specific change is part of. So I would ask again to split them up please?

Phew, lots of text. Does that make sense? Are you d'accord?

[oojacoboo]

My main point regarding the types, was directed more towards how Doctrine handles this:

https://www.doctrine-project.org/projects/doctrine-dbal/en/4.4/reference/types.html#mapping-matrix

You can also override these. This isn't that dissimilar to Perpl's type. I fully understand how the mapping is handled and that they're entirely separate concerns.

type, which defines what code should be produced in model/query/tablemap classes and migrations. It implicates phpType and sqlType, but they can be overridden.

What's wrong with using the type value to determine intent then, if it can be overridden?

I think you argue that a type of ENUM(_NATIVE) should use a DB column type of ENUM and generate PHP code that uses PHP enum?
But should it though?

What happens if you say type="ENUM_NATIVE", but do not specify the actual PHP enum in phpType?

It generates the native PHP enum in the Perpl model. I thought I mentioned this in my last comment, but I think I removed it when I was making changes. This is still needed. Perpl needs to generate enums. The phpType defined with ENUM_NATIVE simply says, "I have my own - use it instead".

What do you do when you want to use the column type ENUM, but just use a string on PHP side?

You enroll yourself into a mental institution. But seriously, what's the point? Is this just for some BC reasons? If so, don't use enums at all. Set the type to varchar and the sqlType to ENUM_NATIVE. As you said, that's the point of overriding these. It seems like you're trying to make the assumption that somehow PHP enums aren't of the highest value and that, somehow, passing around string values, should be the default.

To me, it seems obvious that the default phpType has to be string, as it is the only one that can safely be used in all circumstances.

Absolutely not - wrong in fact. There is nothing "native" about using a string on the PHP side when the underlying database storage is using an enum.

With all that, I hope it becomes clear why I think you are doing three things in this PR:

make phpType work with PHP enums

Yes, this PR covers this.

make type="ENUM(_NATIVE)" create usable migrations on Postgres when combined with sqlType

There isn't any combination with sqlType here, only implied.

work toward tightly coupling PHP enum and SQL ENUM

Absolutely, this should be expected!

Postgres does not actually allow you to use ENUM, it allows you to use a custom type that might - or might not - be based on ENUM; it is important to understand that there is a difference between the two). I think it became clear that this is more than just flipping a switch in PgsqlPlatform and be done with it.

Do not agree. You clearly use MySQL only and not Postgres. Your bias is shining through in your comments very strongly. I used to use MySQL, then switched to Postgres. I used MySQL for probably 20 years. I'd never switch back to MySQL, now, having switched to Postgres and knowing what I do. I'm not trying to make this into some kind of MySQL vs Postgres comparison. But, to say that, just because Postgres implements enums differently from MySQL, that there isn't a native implementation, is very wrong. In fact, Postgres' implementation for enums is far superior. For one, they can be shared across the schema. But you also don't have to drop and rebuild them to alter. MySQL's implementation is more like a value set constraint than anything.

Regardless, the implementation for Postgres' enums is done in this PR.

The third one I think is not feasible, since the way DB type and PHP type are connected is more complex than that and it makes the configuration in schema less expressive (as I hope became clear in the above)

100% disagree. In fact, to not do it is more confusing and incorrect. Can you please explain yourself more here - or maybe we can create another issue ticket to discuss this, since generating the enums isn't part of this PR. But the implementation is dead simple. There is nothing complex about it, as far as I can see. IT doesn't make the configuration of the schema less expressive either.

One way or another, creating the Postgres migration code affects different classes than making phpType work. The two are actually not related at all, once you discard an "implicit" coupling of the two in the mind of a developer.

Unfortunately, they are very related. Take this bit:

type, which defines what code should be produced in model/query/tablemap classes and migrations. It implicates phpType and sqlType, but they can be overridden.

I tried explaining this in my previous comment, but maybe I didn't do a good job.

When you define phpType as a PHP enum, Perpl will try to instantiate that as an object (new WhateverThing($columnValue)). PHP enums are not accessed like this. You have to call WhateverThing::from($columnValue) or WhateverThing::tryFrom($columnValue).

So, without the schema type value telling you it's an ENUM(_NATIVE), you have no way to know how to properly handle the phpType as an enum. There are 2 ways to address this concern.

1. You can rely on the type column to tell you intent (proper way IMO)
2. You can use introspection and then instantiate it accordingly. This is fine, we can do this, but it's less explicit IMO, and won't actually work for all column type values. For instance, you can't access an enum value/name from a bool value, maybe a float and others as well. Typically an enum will have backed values of string or int. Again though, I'm fine with doing it this way and just throwing exceptions. I don't see this as "wrong", just less explicit.

So, that all said, these two attributes (column type and phpType) are intrinsically connected. We can disconnect the phpType and allow for more implicit handling if you insist. Even though I disagree, and think ENUM_NATIVE should have a much more broad application (PHP and DB). In fact, I think it's FAR more important for declaring the PHP side, with regards to generation, than the database side. The database side can easily be overridden using sqlType. The PHP side has no other way to tell Perpl that we need to generate "native" PHP enums (as we should be doing).

[mringler]

What are you trying to archive with statements like

Your bias is shining through in your comments very strongly.

?

It seems like you want to make me support your feature ideas (never formally outlined, discussed, or agreed on) by being argumentative in an unjustified tone. I don't see to what this is meant to succeed.

[oojacoboo]

What are you trying to archive with statements like

Your bias is shining through in your comments very strongly.

Sorry if that seems too direct. However, that's exactly how I'm reading your comments around Postgres' support for enums. Maybe you can better explain where you're coming from here. I still do not understand your line of thinking, and have only been able to resolve it to a misunderstanding, or bias because of your own personal use of MySQL. At the moment, Perpl supports enum database types for MySQL only, as if MySQL is the only DB that actually has enums. Do you feel that's correct, that only MySQL supports enums "natively"?