Skip to content

Conversation

@ryotamoriyama
Copy link

・ACF設定用functionファイルの設置
・env-sampleのデフォルトwpのバージョンを4.9.5にアップデート

@ryotamoriyama ryotamoriyama requested a review from a team May 14, 2018 06:12
@ktogo
Copy link
Contributor

ktogo commented May 14, 2018

@ryotamoriyama Basically the strategy itself you'd like to achieve is fine,

though I would like you to:

  1. Separate this to two PRs by their demands - One for updating the default Wordpress version, the other one for adding a new helper function.
  2. Apply a proper PR name (unlike meaningless name such as Develop)

Thanks!

* 2018.05.12
* author:ryota moiryama
*/
function my_acf_json_save_point($path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not enclosing this into AcfCustomJsonData class?

* 2018.05.12
* author:ryota moiryama
*/
function my_acf_json_load_point($paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

* 2018.05.12
* author:ryota moiryama
*/
function display_acf_option_page()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -0,0 +1,58 @@
<?php

class AcfCustomJsonData
Copy link
Contributor

Choose a reason for hiding this comment

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

The classname should be more clear to tell it's responsibility.

{
private static $acf_custom_json_data;

private static $theme_path = '/inc/acf-json';
Copy link
Contributor

Choose a reason for hiding this comment

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

theme_path seems like pointing to the path of the theme itself.

{
}

public static function get_json_data_path($default_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not have such a global access modifier.

Please make it private along with moving my_acf_json_save_point and other functions into this class.

public static function get_json_data_path($default_path)
{
$path = get_stylesheet_directory() . self::$theme_path;
if (!file_exists($path)) mkdir($path);
Copy link
Contributor

Choose a reason for hiding this comment

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

mkdir should be run recursively using third param arg.

WP_ROOT=/var/www/html
WP_URL=http://localhost
WP_VERSION=4.8.2
WP_VERSION=4.9.5
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be made on different PR since they are completely unrelated.

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.

3 participants