preaction / Statocles

Static website CMS
http://preaction.me/statocles
Other
84 stars 33 forks source link

On creating site, Statocles tries to log before log object exists #563

Closed wbazant closed 4 years ago

wbazant commented 6 years ago

This was an obstacle when I wanted to try out the project, this is a report and a patch I did to push the project on.

I'm creating a site. Stuff breaks with this error:

Can't call method "log" on an undefined value at lib/Statocles.pm line 171, <STDIN> line 4.

This was the stack trace when I ran it under debugger:

@ = DB::DB called from file 'lib/Statocles.pm' line 171
$ = site::log('site') called from file 'lib/Statocles/Command/bundle.pm' line 48
. = Statocles::Command::bundle::bundle_theme('Statocles::Command::bundle', 'default', 'theme') called from file 'lib/Statocles/Command/create.pm' line 129
$ = Statocles::Command::create::create_site(ref(Statocles::Command::create), ref(ARRAY)) called from file 'lib/Statocles/Command/create.pm' line 12
$ = Statocles::Command::create::run(ref(Statocles::Command::create), 'test-site') called from file 'lib/Statocles.pm' line 76
$ = Statocles::run('Statocles', 'create', 'test-site') called from file 'bin/statocles' line 18

I see lib/Statocles/Site.pm does this in BUILD:

$Statocles::SITE = $self;

I also see that Statocless::Command wire up an object site of that class, but as far as I can tell this doesn't happen for me. I don't know if it's a bug or if I haven't installed it right. I ran this with Perl 5.18, and I see the Travis build is failing for it currently (I had the same failing test, and installed with -f), so maybe it's connected somehow.

I've patched it for myself in Statocles.pm as follows:

our $SITE;

BEGIN {
    package # Hide from PAUSE
        site;
    sub log {
        use Mojo::Log;
        our $log = Mojo::Log->new;
        return $log;
    }
}
preaction commented 6 years ago

Thanks for the detailed report!

This is most definitely a bug, and I think I'm going to take your solution and go a bit further: We don't need the Statocles::Site to be a singleton, we really could make it a Statocles object, put a Mojo::Log object on that, and then make a Statocles->log method that does two things:

  1. Called with no arguments, returns the Mojo::Log object. This allows for objects to cache this object close to be able to easily do $self->log.
  2. Called with two arguments ($level and $message), writes to the log.

So every place that now has site->log->debug(...) will be changed to Statocles->log( debug => ... ). This removes the site pseudo-package that is probably super-dangerous to have, and makes for an easy, identifiable, OO-pure(-er) way to get at the log singleton for the entire process.

jjatria commented 4 years ago

Any chance this will get addressed? It's been more than a year, and currently it means new users using the default options cannot create a site. If this is blocked on some other major re-factor, maybe we could just temporarily comment-out the logging line so the create command is not broken.

wbazant commented 4 years ago

Doug is making a v2, see the other branch.

preaction commented 4 years ago

The current code in master is definitely broken. Since I'm working on the v2 branch, I just realized that I can release bugfixes from the old code before I started refactoring (I've pushed it as the 'v0' branch). I'll release a new version right now.