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

UF 4.2.0 changes to migration naming convention should only include PHP files #965

Closed
elliottpost opened this issue Apr 13, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@elliottpost
Copy link

commented Apr 13, 2019

Since UF 4.2.0, the migration naming convention has been relaxed. This change means that any file inside the migration directories will be picked up by the migrator. I would suggest that since the migrator is expected to only look for PHP classes, that it limits the search pattern to include only PHP files. If the migrator encounters a SQL file or CSV file (that our project was using for seeding and raw DB queries for updating tables with enum, for instance [which Eloquent doesn't support]), it gives an error like:

 [ERROR] Unable to find the migration class
         '\UserFrosting\Sprinkle\CardManager\Database\Migrations\v20171115\brand_ids'.

We are unable to run any migrations since updating to 4.2.0 without moving any migration helper files out of the migration directories and updating the old migrations to find a new path. It's not preventing us from working since a work-around is known, but keeping the data files for migrations in the same directory as the migration itself would be helpful.

You can recreate this by adding any non-PHP file to any migration subdirectory.

@elliottpost

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

Quick update to note that I recognize seeds no longer should live under migrations, but I would still argue that if the migrator is expecting PHP classes it should only look at class files, which allows more flexibility with the migration tool itself to allow helper assets.

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

That's indeed a good idea. We could have any king of garbage files in there (.gitkeep, .DS_STORE) that could break everything.

@elliottpost

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

It looks like the best place to approach this would be with the UserFrosting\UniformResourceLocator\ResourceLocatorInterface and UserFrosting\UniformResourceLocator\ResourceLocator to accept some sort of glob() like filter that would add it to the $list[] array in this snippet https://github.com/userfrosting/UniformResourceLocator/blob/master/src/ResourceLocator.php#L420-L427

I see you're the author of that package, but I recognize you're quite busy with the rest of UF as well. If I get a chance over the next week or so I'll try submitting a PR for this myself. Might be a good chance to get a better understanding of the file system tool used in UF.

@elliottpost

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

PR added for URL repository at userfrosting/UniformResourceLocator#9.

Another PR will need to be submitted for UF core's migration system to add blacklisted common extensions (I assumed a blacklist would be more better for all schemes, rather than a whitelist), and a config could easily be added to account for blacklisting by sprinkle, like:
migration.blacklistedExtensions = ['csv', 'json', ...]

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

In this case, I guess URI should have both options (white and blacklist). If we want only to load php file, we don't want to blacklist all of the other don't we? We could have a .png in there that might break things...

@elliottpost

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

I see your point. I have to leave town for a couple weeks but if this is still open when I come back I'll follow-up on the original PR (above) with your recommendations/thoughts and try again.

@lcharette lcharette added this to the 4.2.x milestone Apr 19, 2019

xrobau added a commit to xrobau/UserFrosting-1 that referenced this issue Jun 9, 2019

Fixes userfrosting#965 - Ignore files that don't end in .php
This ignores any files in Migrations that do not end in .php.
It causes problems when scripts, or data, or documentation is
left in the Migrations directory, and the migration task assumes
that they are PHP Classes and crashes.

Signed-Off-By: Rob Thomas <xrobau@gmail.com>

xrobau added a commit to xrobau/UserFrosting-1 that referenced this issue Jun 9, 2019

Fixes userfrosting#965 - Ignore files that don't end in .php
This ignores any files in Migrations that do not end in .php.
It causes problems when scripts, or data, or documentation is
left in the Migrations directory, and the migration task assumes
that they are PHP Classes and crashes.

Signed-Off-By: Rob Thomas <xrobau@gmail.com>
@xrobau

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

#998 resolves this in a simple way,

@lcharette lcharette modified the milestones: 4.2.x, 4.3.0 Jun 11, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

While I like the whitelist/blacklist option better, for a short term solution #998 can indeed solve this. I'll leave this one open, as it's more of a long term solution.

lcharette added a commit that referenced this issue Jun 13, 2019

Merge pull request #998 from xrobau/fix-database-migration
Fixes #965 - Ignore files that don't end in .php
@lcharette

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

See comment here : userfrosting/UniformResourceLocator#9

Initial issue should be solved in next version by #998.

@lcharette lcharette closed this Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.