themehybrid / hybrid-core

Official repository for the Hybrid Core WordPress development framework.
GNU General Public License v2.0
687 stars 143 forks source link

HYBRID_DIR constant... #181

Closed saas786 closed 3 years ago

saas786 commented 3 years ago

Do you think we can drop it?

as my use case is, I am planning to use it alongside HC v4, but it causes issue with the constant defined in HC v4.

In HC v5 & v6 only use case is:

defined here: https://github.com/themehybrid/hybrid-core/blob/35931bb03f5325be244c7d544735ab2663ca3227/src/bootstrap-hybrid.php#L19

and used here: https://github.com/themehybrid/hybrid-core/blob/35931bb03f5325be244c7d544735ab2663ca3227/src/Core/Application.php#L101

and in turn here: https://github.com/themehybrid/hybrid-core/blob/35931bb03f5325be244c7d544735ab2663ca3227/src/functions-helpers.php#L63

Any feedback on this, before I remove it? and provide alternative for: $this->instance( 'path', untrailingslashit( HYBRID_DIR ) );

to something like: $this->instance( 'path', untrailingslashit( __DIR__ ) );

additionally can add note in doc, so users can change it to their liking if needed... like this

https://github.com/justintadlock/nova/blob/3e329e14bfc9dd5728fbdd14753bf7e4aa5377d6/index.php#L16

justintadlock commented 3 years ago

I have a couple of use cases where I need to set the path from within a plugin but the Application class is loaded via the theme. That's the primary reason for the constant.

I'm not against renaming it to HYBRID_PATH to differentiate it between HC6 and previous versions though. I think that might help for your use case too.

justintadlock commented 3 years ago

In Application::registerDefaultBindings(), we can also allow the other method by checking:

if ( ! $this->has( 'path' ) ) {
    $this->instance( 'path', untrailingslashit( HYBRID_PATH ) );
}
saas786 commented 3 years ago

As constant is only used once, I think it should be removed all together.

Yes, we can set the path only if not defined by plugin or theme.

And as path is set in Application::registerDefaultBindings(), we can set a relative path to HC /src by $this->instance( 'path', untrailingslashit( __DIR__ . '/..' ) );

if not defined by user.

Renaming doesn't seem like a better option, as in future version it might conflict again between frameworks :)

saas786 commented 3 years ago

@lkraav

Any thoughts?

justintadlock commented 3 years ago

The renaming was a suggestion to help with your use case. I still need the constant (or a constant) for now. It doesn't cost us anything keeping it around.