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

Update to Font Awesome 5 (Develop) #957

Merged
merged 21 commits into from
Jul 17, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php
/**
* UserFrosting (http://www.userfrosting.com)
*
* @link https://github.com/userfrosting/UserFrosting
* @copyright Copyright (c) 2019 Alexander Weissman
* @license https://github.com/userfrosting/UserFrosting/blob/master/LICENSE.md (MIT License)
*/

namespace UserFrosting\Sprinkle\Account\Database\Migrations\v430;

use Illuminate\Database\Schema\Blueprint;
use UserFrosting\Sprinkle\Core\Database\Migration;

/**
* Groups table migration
* Changes the `icon` column property of `default` to NULL to align with new Font Awesome 5 tag convention.
* Version 4.3.0
*
* See https://laravel.com/docs/5.4/migrations#tables
* @author Alex Weissman (https://alexanderweissman.com)
*/
class UpdateGroupsTable extends Migration
{
/**
* {@inheritdoc}
*/
public static $dependencies = [
'\UserFrosting\Sprinkle\Account\Database\Migrations\v400\GroupsTable'
];

/**
* {@inheritdoc}
*/
public function up()
{
if ($this->schema->hasTable('groups')) {
$this->schema->table('groups', function (Blueprint $table) {
$table->string('icon', 100)->default('NULL')->change();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be null, or an empty string? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with NULL based off this post: https://stackoverflow.com/questions/38351498/remove-default-in-migration

I also compared to the other columns in the table that did not have a default set when the migration ran, and they all showed NULL for the Default property. In mysql here is how the results of my testing:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be possible to do an export of the create table logic to see if the default value was truly reversed (the cleaner the better, leftovers attract edge cases).

Assuming I remember, I'll take a look at what the DDL ends up being.

Copy link
Contributor Author

@amosfolz amosfolz Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is how "default" migration from \UserFrosting\Sprinkle\Account\Database\Migrations\v400\GroupsTable.php looks
userfrosting groups default

Here is the result of proposed UpgradeGroupsTable.php migration up
UpdatedUP

Here is the result of proposed UpgradeGroupsTable.php migration down
updatedDOWN

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the posted DDL is wrong. Or its mislabeled? I'll still have to look at this myself, but it works as a good reminder none-the-less, so thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious about what is wrong?

Other than ('fas fa-user') in this line, the down method returns the table to the 'original' state. I was not certain how to handle this but I thought it would be better to set the default to use the new fas convention rather than the fa.

I relabeled my pictures above that might help clear up any confusion.

});
}
}

/**
* {@inheritdoc}
*/
public function down()
{
$this->schema->table('groups', function (Blueprint $table) {
$table->string('icon', 100)->default('fas fa-user')->change();
});
}
}