Skip to content

Conversation

@szepeviktor
Copy link

@szepeviktor szepeviktor commented Mar 20, 2023

@Clorith What do you think about autoloading classes?

source

@vikasiwp
Copy link
Contributor

vikasiwp commented Jun 2, 2023

Sorry, we have taken over from @Clorith , we will review this and get back to you.

@Clorith
Copy link
Contributor

Clorith commented Jun 13, 2023

Adding an autoloader here definitely makes sense, there are a lot of potential cross-referenced dependencies, and this would both help to maintain a cleaner code base, and avoid accidental errors in the future if a dependency file is not properly required.

I also appreciate the inclusion of a custom autoloader file, over using Composer, which I've seen happen a few times elsewhere and isn't always as elegant within a WordPress plugin.

Some elements may have been unintentionally left out from a quick glance at the changelist, for example the https://github.com/szepeviktor/InstaWP_wordpress-string-locator/blob/2aa88c809ced2b2a9716ee4c7308822aeca42d08/includes/extension/searchreplace/class-replace.php#L99 call to initiate the SearchReplace extension of the plugin won't trigger, so a few files will still need ot be manually called.

The includes/extension directory was intended for additional functionality for the plugin, which could be dropped in/out depending on the plugins future needs, so perhaps the right thing here would be to give each entry in this directory its own autoloader?
This would mean 3 autoloaders (at this time) in the plugin, but would maintain that portability, and would ensure the extensions load as intended since the entry file for each of them would still need to be called form the primary plugin file, how does that approach sound?

@szepeviktor
Copy link
Author

how does that approach sound?

All right!
I'm not a user of your project.
This is really an inspiration, not a PR :)

Could you implement the proposed changes?

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