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

Add Listener to PreAssetsCompileEvent #22

Conversation

WebMamba
Copy link
Contributor

No description provided.

@WebMamba WebMamba changed the title Add Listener to PreAssetsCompileEventListener Add Listener to PreAssetsCompileEvent Oct 23, 2023
@WebMamba WebMamba force-pushed the add_listener_to_pre_assets_compile_event branch from 9ab7aa2 to 7cdba96 Compare October 23, 2023 12:37
@WebMamba
Copy link
Contributor Author

The PublicAssetsPathResolverInterface had some changes on the last release of AssetMapper. I'm not sure how we should handle it here. See the error on phpstan. Should we just remove the getPublicFilesystemPath from the SassPublicPathAssetPathResolver? What do you think @weaverryan?

@weaverryan
Copy link
Contributor

I see it. Could we surround that with a method_exists? Would that do it?

public function getPublicFilesystemPath(): string
{
    if (!method_exists($this->decorator', 'getPublicFilesystemPath')) {
        throw new \Exception('...');
    }

    // rest of method
}

@WebMamba
Copy link
Contributor Author

Damm didn't know about this trick to fool phpstan! Thanks! 😁

@WebMamba
Copy link
Contributor Author

@bocharsky-bw I do not have the right to merge this PR, and I think this is ready to go 😁

@bocharsky-bw
Copy link
Member

Sorry for the delay, I hoped for @weaverryan final review on it.

Thanks!

@bocharsky-bw bocharsky-bw merged commit b97c274 into SymfonyCasts:main Nov 14, 2023
10 checks passed
->set('sass.public_asset_path_resolver', SassPublicPathAssetPathResolver::class)
->decorate('asset_mapper.public_assets_path_resolver')
->args([
service('.inner')
])

->set('sass.listener.pre_assets_compile', PreAssetsCompileEventListener::class)
Copy link

@ogizanagi ogizanagi Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument is missing:

            ->args([
                service('sass.builder')
            ])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds valid. Would you like to create a PR?

//cc @WebMamba

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.

None yet

4 participants