Errors with RockMigrationsConstants Traits

[gebeer]

Hi @bernhard,

I already fell in love with the new config migrations feature in RockMigrations > 6.0.0. I am currently on v6.0.1

So much cleaner to have migrations organized in this way.

I just tried to implement the part about the constant traits, described at https://www.baumrock.com/en/processwire/modules/rockmigrations/docs/config-migrations/#class-constant-traits

First it would not work at all as described in the docs. I noticed that my file at site/modules/Site/RockMigrations/RockMigrationsConstants.php never got picked up because of searching for those files did not recurse into module subdirectries. Fixed in PR#70 commit  https://github.com/baumrock/RockMigrations/pull/70/commits/faba5475a008ab14415c0e35f2c5c78d2e91f0b1.

First problem:

After fixing that, I get the Compile Error
Traits cannot have constants

Actually, constants are only allowed in Traits since PHP 8.2.

So this would change the module dependencies from PHP>=8.0 to PHP>=8.2

I don't think this was intended. But it should be updated or at least documented that the automated constants thingy requires PHP>=8.2.

Second problem:

After upgrading to PHP8.2 I ran into the next issue:

This is caused by the constant name "template_basic-page" containing a hyphen. As it seems, they are not allowed in constant names in PHP at all (https://www.php.net/manual/en/language.constants.php):

Quote

The name of a constant follows the same rules as any label in PHP. A valid constant name starts with a letter or underscore, followed by any number of letters, numbers, or underscores. As a regular expression, it would be expressed thusly: ^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$

For years I have naming conventions for templates and fields in PW. Template names always contain hyphens like in "basic-page". Field names always contain underscores like in "my_field". This is also helpful when looking at code to discern templates from fields on the first glance.

I fixed this by replacing - with _ in constant names. See https://github.com/baumrock/RockMigrations/pull/70/commits/a06c6062c61b5264932a8044cf25bac0c689f8cf

The issue with PHP8.2 still remains to be resolved. I suggest adding the requirement only to docs  about RockMigrationsConstants.php because other than that RockMigrations seemed to work fine on 8.1 in my tests.

 

[bernhard]

Hey @gebeer thanks for trying out Config Migrations so early 🙂 💪

10 hours ago, gebeer said:

I already fell in love with the new config migrations feature in RockMigrations > 6.0.0.

Same here 😍 

10 hours ago, gebeer said:

First it would not work at all as described in the docs. I noticed that my file at site/modules/Site/RockMigrations/RockMigrationsConstants.php never got picked up because of searching for those files did not recurse into module subdirectries. Fixed in PR#70

The file should not be in /site/modules/Site/RockMigrations/... but should live side by side with Site.module.php! I just had a look into the docs and you are right - the instructions there are like you say. I have to look into what you said tomorrow, but if you have time and read this in the meantime, then I'd appreciate if you could try to create the file /site/modules/Site/RockMigrationsConstants.php and see if that maybe fixes some other of your mentioned issues?

10 hours ago, gebeer said:

The issue with PHP8.2 still remains to be resolved. I suggest adding the requirement only to docs  about RockMigrationsConstants.php because other than that RockMigrations seemed to work fine on 8.1 in my tests.

Thx a lot! I'll likely do exactly that 🙂 As of 2 days ago we have PHP8.4 released and PHP8.1 gets security fixes only - so I think it would even be fine to have PHP8.2 as a requirement for the module. Any other votes?

[gebeer]

7 hours ago, bernhard said:

hen I'd appreciate if you could try to create the file /site/modules/Site/RockMigrationsConstants.php and see if that maybe fixes some other of your mentioned issues?

This solves only the first problem that I described.

The error caused by hyphens in constant names still persists, even after switching to PHP8.3. Do you name all your templates with underscores like "basic_page" to avoid that?

On a side note, after reading up on PHP constants, there seems to be a convention to name them in capital letters.

[bernhard]

7 hours ago, gebeer said:

The error caused by hyphens in constant names still persists, even after switching to PHP8.3. Do you name all your templates with underscores like "basic_page" to avoid that?

Yes I always use underscores. I had problems with hyphens several times. I can't remember any more where exactly, but I remember that avoiding them was the easiest solution.

7 hours ago, gebeer said:

On a side note, after reading up on PHP constants, there seems to be a convention to name them in capital letters.

Any sources for that? PW definitely uses camelcase constants like Inputfield::collapsedHidden which I think looks nicer than Inputfield::COLLAPSEDHIDDEN. But I'd be happy to get some links to read.

[gebeer]

6 minutes ago, bernhard said:

Any sources for that? PW definitely uses camelcase constants like Inputfield::collapsedHidden which I think looks nicer than Inputfield::COLLAPSEDHIDDEN. But I'd be happy to get some links to read.

https://www.perplexity.ai/search/php-constants-naming-conventio-g4mLG8sbQyu34r4mGXmx6w 

Although the source for this is not php.net directly. I wanted to read up on that over there but php.net currently throws a 502 error.

Anyways, that was just a side note.

8 minutes ago, bernhard said:

Yes I always use underscores

That explains why you never saw that error. But imo you should not assume that everybody uses underscores so I think, the renaming logic in my PR at https://github.com/baumrock/RockMigrations/pull/70/commits/a06c6062c61b5264932a8044cf25bac0c689f8cf should be implemented.
Otherwise people who use hyphens in their template names would have to rename all their templates just to be able to use the RockMigrationsConstants magic.