Autoload directories #841

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
@jamierumbelow
Contributor

jamierumbelow commented Dec 27, 2011

There are times in a developer's life where we need to load a bunch of arbitrary files, either when we're working with third-party code or when we want to extend out our application past the basic set of libraries and helpers. For example, presenters (see the blog post) would really benefit from being able to be automatically loaded.

This can also be useful when wanting to load all the files in a directory that might change regularly -- so maintaining an array to load everything would be tiresome.

I've alleviated this problem by adding a $this->load->directory() method to the Loader, as well as adding an $autoload['directories'] type to the autoload.php file. I'm using glob() to do the searching, so devs can use rather complex patterns to get nice and specific.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Dec 29, 2011

Contributor

👍

Contributor

timw4mail commented Dec 29, 2011

👍

+ * @return void
+ */
+ public function directory($directory, $recursive = FALSE)
+ {

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 3, 2012

Contributor

I'd like to suggest a slightly improved version:

$files = glob($directory, GLOB_MARK);
$filecount = count($files);

if (is_array($files) && $filecount > 0)
{
    for ($i = 0; $i < $filecount; $i++)
    {
        if ($recursive && is_dir($files[$i]))
        {
            $this->directory($files[$i].'*', TRUE);
        }
        elseif (is_file($files[$i]) && @is_readable($files[$i]))
        {
            @include($files[$i]);
        }
    }
}

Notes:

  • GLOB_MARK will make sure that there's a trailing DIRECTORY_SEPARATOR for each directory listed (if any).
  • for() should be slightly faster than foreach(), and since it uses count($files) as well - we should pre-cache it in $filecount.
  • We don't need to nest single if()s when there's no other logic, hence merging the recursivity and is_dir() checks.
  • is_file() does the trick, but if we already know that it's a directory - why go there at all? elseif() is more suitable. We also don't need to try including a file that we can't read, so we put is_readable() as well and suppress any warnings - just in case.
@narfbg

narfbg Jan 3, 2012

Contributor

I'd like to suggest a slightly improved version:

$files = glob($directory, GLOB_MARK);
$filecount = count($files);

if (is_array($files) && $filecount > 0)
{
    for ($i = 0; $i < $filecount; $i++)
    {
        if ($recursive && is_dir($files[$i]))
        {
            $this->directory($files[$i].'*', TRUE);
        }
        elseif (is_file($files[$i]) && @is_readable($files[$i]))
        {
            @include($files[$i]);
        }
    }
}

Notes:

  • GLOB_MARK will make sure that there's a trailing DIRECTORY_SEPARATOR for each directory listed (if any).
  • for() should be slightly faster than foreach(), and since it uses count($files) as well - we should pre-cache it in $filecount.
  • We don't need to nest single if()s when there's no other logic, hence merging the recursivity and is_dir() checks.
  • is_file() does the trick, but if we already know that it's a directory - why go there at all? elseif() is more suitable. We also don't need to try including a file that we can't read, so we put is_readable() as well and suppress any warnings - just in case.

This comment has been minimized.

Show comment Hide comment
@ericbarnes

ericbarnes Jan 4, 2012

Contributor

@jamierumbelow @narfbg I would prefer this to use an existing directory helper like get_dir_file_info(). Since the functionality is already in the core and would prevent the duplication.

@ericbarnes

ericbarnes Jan 4, 2012

Contributor

@jamierumbelow @narfbg I would prefer this to use an existing directory helper like get_dir_file_info(). Since the functionality is already in the core and would prevent the duplication.

This comment has been minimized.

Show comment Hide comment
@jamierumbelow

jamierumbelow Jan 4, 2012

Contributor

Semantically, I'm actually quite hesitant, if I'm honest, to even call it directory, when in actual fact, due to how glob() works, you're specifying the files, not the directory. Would calling it $this->load->files() cause confusion with $this->load->file()?

@ericbarnes You're spot on -- I'll get it converted over to get_dir_file_info() this afternoon. Never noticed that -- I was looking in the dir helper not the file helper :)
@narfbg Great additions to my shoddy code. Thanks a bundle.

@jamierumbelow

jamierumbelow Jan 4, 2012

Contributor

Semantically, I'm actually quite hesitant, if I'm honest, to even call it directory, when in actual fact, due to how glob() works, you're specifying the files, not the directory. Would calling it $this->load->files() cause confusion with $this->load->file()?

@ericbarnes You're spot on -- I'll get it converted over to get_dir_file_info() this afternoon. Never noticed that -- I was looking in the dir helper not the file helper :)
@narfbg Great additions to my shoddy code. Thanks a bundle.

@AkenRoberts

This comment has been minimized.

Show comment Hide comment
@AkenRoberts

AkenRoberts Jan 24, 2012

Contributor

Somewhat related to this (and Jamie's post on view presenters) - what are your thoughts on using "require_once()" versus adding another method to the Loader class that basically does the same thing? Has anyone noticed a preference, or complaints about one or the other?

I personally would think that replacing a require_once() near the top of my file with something along the lines of $this->load->class('product'); would better suit the CodeIgniter coding style.

I like the idea of this pull request 👍, but I can see myself and most likely others not wanting to load unnecessary files, nor separate classes into directories in order to only load one file, even though it may be related to others.

Contributor

AkenRoberts commented Jan 24, 2012

Somewhat related to this (and Jamie's post on view presenters) - what are your thoughts on using "require_once()" versus adding another method to the Loader class that basically does the same thing? Has anyone noticed a preference, or complaints about one or the other?

I personally would think that replacing a require_once() near the top of my file with something along the lines of $this->load->class('product'); would better suit the CodeIgniter coding style.

I like the idea of this pull request 👍, but I can see myself and most likely others not wanting to load unnecessary files, nor separate classes into directories in order to only load one file, even though it may be related to others.

@AkenRoberts

This comment has been minimized.

Show comment Hide comment
@AkenRoberts

AkenRoberts Oct 13, 2012

Contributor

Someone's gotta bump this stuff! 👍

While re-reading this, perhaps instead of directory() or files(), you could use php(), class() (as mentioned), or an "action" name, such as include() or require().

Given the differences between include/require and *_once(), maybe have both available, either as separate methods or optional parameter(s).

I'd like to hear other opinions on this subject. I can imagine people don't want to create unnecessary libraries just to wrap a class they wish to require into an easy-to-load-by-CI format.

Contributor

AkenRoberts commented Oct 13, 2012

Someone's gotta bump this stuff! 👍

While re-reading this, perhaps instead of directory() or files(), you could use php(), class() (as mentioned), or an "action" name, such as include() or require().

Given the differences between include/require and *_once(), maybe have both available, either as separate methods or optional parameter(s).

I'd like to hear other opinions on this subject. I can imagine people don't want to create unnecessary libraries just to wrap a class they wish to require into an easy-to-load-by-CI format.

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Dec 16, 2014

Contributor
Contributor

ivantcholakov commented Dec 16, 2014

@AkenRoberts

This comment has been minimized.

Show comment Hide comment
@AkenRoberts

AkenRoberts Dec 17, 2014

Contributor

Ugh, I actually suggested that two years ago?!

Contributor

AkenRoberts commented Dec 17, 2014

Ugh, I actually suggested that two years ago?!

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Dec 17, 2014

Contributor

I am trying to understand how useful this idea is. Presenters were menioned as a particular (but not only) use case, this is why I updated the link.

Offtopic: There is an interesting development on presenters https://github.com/druu/ci-presenter which relies on a library.

The suggestion by @jamierumbelow is ok, I think about adding some value to it. Somehow a liitle magic for not forcing me as a developer to deal with constants as BASEPATH, APPPATH, FCPATH would be nice, but I don't hardly insist on this.

Contributor

ivantcholakov commented Dec 17, 2014

I am trying to understand how useful this idea is. Presenters were menioned as a particular (but not only) use case, this is why I updated the link.

Offtopic: There is an interesting development on presenters https://github.com/druu/ci-presenter which relies on a library.

The suggestion by @jamierumbelow is ok, I think about adding some value to it. Somehow a liitle magic for not forcing me as a developer to deal with constants as BASEPATH, APPPATH, FCPATH would be nice, but I don't hardly insist on this.

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Dec 17, 2014

Contributor

Actually, the magic I think about is beter not to be injected within the proposed method (and to make it complex), it could be implemented separately.

+1

Contributor

ivantcholakov commented Dec 17, 2014

Actually, the magic I think about is beter not to be injected within the proposed method (and to make it complex), it could be implemented separately.

+1

@jamierumbelow

This comment has been minimized.

Show comment Hide comment
@jamierumbelow

jamierumbelow Dec 17, 2014

Contributor

@ivantcholakov Do check out Composer. It's got a very sophisticated autoloader and can be integrated with CodeIgniter easily.

Contributor

jamierumbelow commented Dec 17, 2014

@ivantcholakov Do check out Composer. It's got a very sophisticated autoloader and can be integrated with CodeIgniter easily.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Dec 17, 2014

Contributor

Indeed, we should encourage usage of Composer for things like this. Closing.

Contributor

narfbg commented Dec 17, 2014

Indeed, we should encourage usage of Composer for things like this. Closing.

@narfbg narfbg closed this Dec 17, 2014

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Dec 17, 2014

Contributor

Ok, I agree.

Contributor

ivantcholakov commented Dec 17, 2014

Ok, I agree.

baypup pushed a commit to baypup/CodeIgniter that referenced this pull request Aug 20, 2015

Merge pull request #841 from msadhrao/2
Auth _render_page function argument changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment