public_dir option does not seem to work #975

Closed
fgabolde opened this Issue Aug 11, 2015 · 21 comments

Projects

None yet

6 participants

@fgabolde

The Dancer2::Config POD documents the public_dir option, but it doesn't seem to do anything.

After looking at the Dancer2::Handler::File code I can't figure out when the Handler object is actually created and registered. In any case, DANCER_PUBLIC seems to work (although undocumented), so it might be an issue of loading the config after the handler is registered.

Steps to reproduce:

$ dancer2 -a MWE::PublicDirDoesNotWork
$ cd MWE-PublicDirDoesNotWork
$ mv public foo
# edit config.yml, add "public_dir: foo"
$ plackup bin/app.psgi
# visit http://localhost:5000 or wherever your Plack::Handler lives: all requests to static assets 404
@plicease
plicease commented Sep 9, 2015

I think this is the problem:

https://github.com/PerlDancer/Dancer2/blob/master/lib/Dancer2/Core/App.pm#L556

The setting static_handler is dependent on the default public directory existing, so when you move public to foo and change the configuration it correctly changes the public dir but turns off the static handler.

My work around for this was to set static_handler => 1.

@fgabolde
fgabolde commented Sep 9, 2015

I see!

That indeed seems likely. It would fit the symptom of working with DANCER_PUBLIC, too.

I'll test your workaround tomorrow at work.

@rleir
rleir commented Oct 1, 2015

Agreed, the workaround works:
static_handler: 1

I am triaging: should we accept the workaround and close this issue, or would you like to pull? Or leave it open a while?

@fgabolde
fgabolde commented Oct 1, 2015

@rleir At the very least, the workaround should be documented before this issue is closed, don't you think?

I can't speak for the devs of course, but I think this should be considered a bug.

If the devs don't have tuits to work on this (it's admittedly not a deal breaker and like you said there's a workaround) I can provide a patch, but it won't be ready any time soon.

@plicease
plicease commented Oct 1, 2015

I agree with everything @fgabolde said in the previous comment.

@sdeseille
Contributor

Hello

I will be happy to update documentation and make pull request to fixe that.
Can I ?

regards

sdeseille

@sdeseille
Contributor

I found public_dir entries in Config.pod and Manual.pod. I made test to validate each case described in documentation.

Do you know if their is another place where public_dir is mentioned ?

@sdeseille
Contributor

I created branch "update_documentation_about_public_dir" in order to propose the pull request.

@cromedome
Contributor

Migration.pod, line 55.

@cromedome
Contributor

While the the workaround seems fair, I agree with fgabolde, I think i's a bug that should be fixed.

I looked through things and if I am following right, the default config is built, ConfigReader reads any additional files and merges their contents with the default config. Nowhere is the test for static_handler rerun (validating plicease's observation). My question is, at what point in the ConfigReader is it appropriate to reset that? My gut tells me at the very end of _build_config(), but would anyone else like to weigh in?

Unrelated, there is a TODO at the end of load_config_file() (it's probably mine) regarding merging config files. As this is now handled in _build_config(), I will take the TODO out if I'm ok to fix the static_handler bug.

Anyone have a preference for where the recheck to set that bit should live?

@sdeseille
Contributor

Thank you @cromedome . I updated Migration.pod. Would you check my branch before i do pull request ?

regards

sdeseille

@cromedome
Contributor

That looks good to me, @sdeseille!

I apologize in advance if you're writing a doc patch for something I end up fixing :) Your changes look good though!

@sdeseille
Contributor

Don't worry. I'm happy ;)

@cromedome
Contributor

I would like to add a _validate_config() method to Dancer2::Core::Role::ConfigReader that looks for situations like this, where changing one setting from default might require us to alter/check/correct/set another setting. For now, this would be the only case that we are checking for, but it gives us some plumbing in case we run across other instances where this might occur.

Thoughts?

@cromedome
Contributor

Forgot to mention, this would get added around _normalize_config() in the existing _build_config() method.

@xsawyerx xsawyerx added the Bug label Nov 8, 2016
@xsawyerx
Member
xsawyerx commented Nov 8, 2016 edited

I also agree this is a bug that needs fixing. I'm tentatively adding this to the next milestone. If we can't get around to it, we'll shift it to the next one.

@xsawyerx xsawyerx added this to the 0.205000 milestone Nov 8, 2016
@xsawyerx
Member

I have a fix for this, but while writing the test, I think I discovered another bug in the code somewhere. I'll try to prepare pull requests for both issues tonight.

@xsawyerx
Member

Scratch the "two bugs" thing. I have a fix and a test.

@xsawyerx
Member

Okay the punk show drained me. Patch incoming today.

@xsawyerx xsawyerx added a commit that referenced this issue Dec 14, 2016
@xsawyerx xsawyerx GH #975: Postpone public_dir calculation:
The problem in GH #975 is that we decide what the public_dir should
be before we read the configuration, which might give a different
public dir.

This can be resolved by deciding this later, during the to_app
time.

The additional problem this exposes is that public_dir created from
both DANCER_PUBLIC and "public_dir" configruation option does not
take into account the location (app dir). I don't think that's
correct. The test shows this in the config.yml file.
ce9820d
@xsawyerx
Member

Alright. Fix provided. This does expose another problem which I'll try to fix as soon as I figure out how.

@cromedome cromedome added a commit that referenced this issue Dec 21, 2016
@xsawyerx @cromedome xsawyerx + cromedome GH #975: Postpone public_dir calculation:
The problem in GH #975 is that we decide what the public_dir should
be before we read the configuration, which might give a different
public dir.

This can be resolved by deciding this later, during the to_app
time.

The additional problem this exposes is that public_dir created from
both DANCER_PUBLIC and "public_dir" configruation option does not
take into account the location (app dir). I don't think that's
correct. The test shows this in the config.yml file.
f2ac574
@cromedome cromedome added a commit that referenced this issue Dec 21, 2016
@cromedome cromedome Merge branch 'fix/gh-975' 0234f4e
@cromedome cromedome added a commit that referenced this issue Dec 21, 2016
@cromedome cromedome v0.204002
    [ BUG FIXES ]
    * GH #975: Fix "public_dir" configuration to work, just like
      DANCER_PUBLIC. (Sawyer X)

    [ ENHANCEMENTS ]
    * You can now call '$self->find_plugin(...)' within a plugin
      in order to find a plugin, in order to use its DSL in your
      custom plugin. (Sawyer X)

    [ DOCUMENTATION ]
    * GH #1282: Typo in Cookbook. (Kurt Edmiston)
    * GH #1214: Update Migration document. (Sawyer X)
    * GH #1286: Clarify hook behavior when disabling layout (biafra)
    * GH #1280: Update documentation to use specific parameter
                keywords (Hunter McMillen)
7675ce1
@xsawyerx
Member

The fix for this was already applied in the previous version. This can be closed.

@xsawyerx xsawyerx closed this Jan 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment