Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set MIME type based on requested asset #202

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

renaatdemuynck
Copy link

@renaatdemuynck renaatdemuynck commented Nov 17, 2016

This fixes issue #200. MIME types are now determined based on the requested asset.
Also fixed some tests that were failing on my Windows machine due to the use of slashes instead of DIRECTORY_SEPARATORs.

@renaatdemuynck renaatdemuynck changed the title Fix for failing tests on Windows Set MIME type based on requested asset Jan 5, 2017
@renaatdemuynck
Copy link
Author

Can this pull request be accepted?

@RWOverdijk
Copy link
Owner

I had a comment here, but I never clicked on comment... It must have been a slow day.

I don't know if I can, it looks very much like it will break for existing implementations (as you had to alter the tests).

@wshafer
Copy link
Contributor

wshafer commented Jul 24, 2017

@renaatdemuynck - If I'm reading this right this changes the responsibility of figuring out the Mime type from the actual file, to the file being requested?

So if I map test.css to real file test.js (not sure why I'd do that but I could) the mime type coming back would be text/css instead of the actual application/javascript for the file?

Personally I'm never a fan of trusting the client but is there some benefit here to letting the client tell you what the server should be serving as the mimetype, verses the actual files themselves?

@wshafer
Copy link
Contributor

wshafer commented Jul 24, 2017

Just saw the open issue here: #200 which answers my question

@RWOverdijk
Copy link
Owner

Merging this would be a bump to 2.0.0. I'm wondering if maybe this behavior should be made configurable instead. Thoughts?

@wshafer
Copy link
Contributor

wshafer commented Jul 26, 2017

@RWOverdijk - I was going go ahead and put this into the v2 branch.

@RWOverdijk
Copy link
Owner

@wshafer I'd prefer that, too.

wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Jul 28, 2017
wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Jul 28, 2017
wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Aug 3, 2017
wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Aug 3, 2017
wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Aug 3, 2017
@renaatdemuynck
Copy link
Author

Sorry, I missed the whole conversation. I just got back from vacation. Thanks for merging this into v2. @RWOverdijk: About the ability to configure, I believe the standard behavior should be the new one. You could make an option to revert to the old behavior for BC reasons, but I don't see a real need for it.

@wshafer
Copy link
Contributor

wshafer commented Aug 24, 2017

This is going to be a major version bump. So I'll just make it the default behavior. It makes more sense because there's another change coming to mimetypes and this fits perfectly with that change as well.

Thanks for the help

wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Sep 21, 2017
wshafer pushed a commit to wshafer/assetmanager-core that referenced this pull request Sep 21, 2017
wshafer added a commit to wshafer/assetmanager-core that referenced this pull request Sep 21, 2017
@thestanislav
Copy link

thestanislav commented Nov 22, 2017

This breaks my code, we can not mix css, less, sass files now :( but we should do it.

// some_config.php
[
    'resolver_configs' => [
        'collections' => [
            'assets/css/base.css' => [
                'assets/less.css',
                'assets/sass.css',
                'assets/other_file.css'
            ],
            'assets/less.css' => [
                'path/less.less',
            ],
            'assets/sass.css' => [
                'path/sass.less',
            ],

    ],
    'filters' => [
        'assets/less.css' => [
            [
                'service' => 'SomeLessCompiler',
            ]
        ],
        'assets/sass.css' => [
            [
                'service' => 'SomeSassCompiler',
            ]
        ]
    ],
]

This code will fail with error "... doesn't have the expected mime-type ...", but logically everything should be fine

[
    'resolver_configs' => [
        'collections' => [
            'assets/base.css' => [
                'assets/less.less',
            ],

    ],
    'filters' => [
        'assets/base.css' => [
            [
                'service' => 'SomeLessCompiler',
            ]
        ],
    ],
]

assets/base.css will output with mime type text/less, but actual type is text/css after compilation

@wshafer
Copy link
Contributor

wshafer commented Nov 22, 2017

Thanks for the report.

I believe I addressed this issue with a different merge for v2, but I’ll test this on my end and make sure this use case is working before I tag it.

@wshafer
Copy link
Contributor

wshafer commented Dec 27, 2017

@thestanislav - I have finally tagged v 2.0.0. I have also confirmed that the following config will work just fine.

<?php

return [
    'asset_manager' => [
        'resolver_configs' => [
            'collections' => [
                'assets/css/base.css' => [
                    'assets/less.css',
                    'assets/scss.css',
                ],
            ],
            'private_collections' => [
                'assets/less.css' => [
                    __DIR__.'/../../assets/less.less',
                ],
                'assets/scss.css' => [
                    __DIR__.'/../../assets/scss.scss',
                ],
            ],
        ],

        'filters' => [
            'assets/less.css' => [
                [
                    'filter' => 'Lessphp'
                ]
            ],

            'assets/scss.css' => [
                [
                    'filter' => 'Scssphp'
                ]
            ],
        ]
    ]
];

To update to the beta version see the upgrade guide here

Let me know if you encounter any other issues.

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.

4 participants