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

Make it possible to define custom acf types #30

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

kevinkiel
Copy link

@kevinkiel kevinkiel commented Apr 25, 2017

Example: corcel.config.php

corcel

@jgrossi
Copy link
Member

jgrossi commented Apr 25, 2017

Hi @kevinkiel thanks for contributing with Corcel ;-)

  • In the case of using a config file I think it's better to add a sample file in corcel/corcel repo, with a CorcelServiceProvider or something like that. Than the user can get a default file with the suggested options;

  • Change the Corcel branch from master to develop, like the contribution guide (done!);

  • Can you fix the error? The phpunit is failing. Please run phpunit locally and ensure nothing is broke.

What do you think? JG

@jgrossi jgrossi changed the base branch from master to develop April 25, 2017 13:12
Copy link
Member

@jgrossi jgrossi left a comment

Choose a reason for hiding this comment

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

@kevinkiel take a look on this. Your commit is associated with an unrecognized email address:

Unrecognized author. If this is you, make sure [email protected] is associated with your account. Click to add email addresses in your account settings.

I suggest you to add [email protected] to your Github account or change your local Git email address. This is important to Github to follow all contributions you did.

@@ -49,65 +49,53 @@ public static function make($name, Model $post, $type = null)
$type = $fakeText->fetchFieldType($key);
}

$default_types = [
Copy link
Member

@jgrossi jgrossi Apr 25, 2017

Choose a reason for hiding this comment

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

Let's move this to an class variable (static):

protected static $defaultTypes = [...];


$types = array_merge($default_types, $custom_types);

if (!empty($types[$type])) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest here:

if (!empty($types[$type])) {
    $field = new $types[$type]($post);
    $field->process($name);

    return $field;
}

return null;

default: return null;
$custom_types = [];

if (\Config::has('corcel.acf.custom_types')) {
Copy link
Member

Choose a reason for hiding this comment

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

You have to check if the Config class exists before using it. That's the first error when running phpunit. I suggest you to check if the class exists or if the config() function exists like I did on Model.php class. Please, take a look.

@kevinkiel
Copy link
Author

@jgrossi Thanks for your fast reaction and feedback. I made some small changes to the code so it would pass the tests.

In the case of using a config file I think it's better to add a sample file in corcel/corcel repo, with a CorcelServiceProvider or something like that. Than the user can get a default file with the suggested options;
I don't understand what you mean. This config file is specifically for the support of custom ACF plugins. Maybe add it in corcel/acf?

@jgrossi
Copy link
Member

jgrossi commented May 2, 2017

What I mean is to create the config/corcel.php file inside the corcel/acf merge an existent config/corcel.php in corcel/corcel. We can merge configurations between files. This way we would have only one config/corcel.php file.

Take a look: https://laravel.com/docs/5.3/packages#configuration

@kevinkiel
Copy link
Author

kevinkiel commented May 2, 2017

@jgrossi Thanks for your response. I've add the CorcelAcfServiceProvider. Now my question is in what package are we gonna merge the 2 config files?

$this->mergeConfigFrom( __DIR__.'/config.php', 'corcel' );

@kevinkiel
Copy link
Author

@jgrossi What do you think?

@jgrossi
Copy link
Member

jgrossi commented Jun 20, 2017

Hi @kevinkiel sorry for being MIA but time is scarce for now :-)

My goal is to have, at least for now, just one config/corcel.php file. I'm rewriting Corcel (you're welcome to help btw) and it'll have a config/corcel.php file, just to set the default Corcel connection, nothing more for now, I guess.

I was wondering if I can merge the config options you created (about acf) into the main config/corcel.php file, what I guess it's possible, but I've never tried that.

I'll try to install a fresh Laravel here, install Corcel, copy the config file and then install corcel/acf and try to merge it. I let you know if it's working and then we merge this PR, which I think is ready.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants