Skip to content

Dependencies update with assetic framework #221

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.

Sign up for GitHub

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

Jump to bottom
Merged
RWOverdijk merged 5 commits into from
Dec 17, 2020
Merged

Dependencies update with assetic framework #221

RWOverdijk merged 5 commits into from
Dec 17, 2020

Conversation

@thomasvargiu
Copy link
Contributor

@thomasvargiu thomasvargiu commented Oct 13, 2020 •
edited
Loading

This branch is based on #219. It updates tests and dependencies.

Changelog

Added

  • Added support for PHP 8.0
  • Added AssetManager\Cache\PsrSimpleCacheAdapter to use a PSR-16 cache implementation for Assetic cache

Changes

  • Migrated to assetic-php/assetic, see 2.0.0 release notes for BC Breaks
  • Bumped up laminas/laminas-stdlib minimum version to 3.2.1
  • AssetManager\Resolver\ResolverInterface should now return Assetic\Contracts\Asset\AssetInterface|null

BC Break

  • Migrated to assetic-php/assetic, see 2.0.0 release notes for BC Breaks.
    The most notable changes used in this is library are:
    • Assetic\Asset\AssetInterface moved in Assetic\Contracts\Asset\AssetInterface
    • Assetic\Asset\FilterInterface moved in Assetic\Contracts\Asset\FilterInterface
    • Assetic\Asset\CacheInterface moved in Assetic\Contracts\Asset\CacheInterface
    • AssetManager\Resolver\ResolverInterface should now return Assetic\Contracts\Asset\AssetInterface|null

@thomasvargiu thomasvargiu changed the title Deps update Dependencies update with assetic framework Oct 13, 2020
@jlaudenbach
Copy link

jlaudenbach commented Dec 14, 2020

Hi,
I have also problems with composer update due to the included kriswallsmith/assetic package (and therefore the restriction to old symfony/process).
When will this MR be accepted? In days or more in month?
I just ask, to think about if it is reasonable to wait, or if I should look for an alternative solution.

Best regards,
Jan Laudenbach

@RWOverdijk
Copy link
Owner

RWOverdijk commented Dec 16, 2020

In its current state it can't be merged (due to conflicts). As to whether or not this is a wanted change: I'm not sure. Judges?

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Dec 16, 2020

I'll try to rebase it

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Dec 16, 2020

@RWOverdijk rebased!

I think a lot of people needs it. Big projects need to be updated, and it's a problem if a library doesn't allow it.
You should release it with a major version.

@RWOverdijk
Copy link
Owner

RWOverdijk commented Dec 16, 2020

Alrighty. I'm down with that. Just pinging @Ocramius so he can stop me if needed.

I'll try to get on that tomorrow at the end of the day.

Ocramius
Ocramius approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM: just beware that tagging requires a major version bump due to signatures changed because of third-party dependency bump cascading into signatures in this package.

src/AssetManager/Resolver/ResolverInterface.php
* @param string $path The path to resolve.
*
* @return \Assetic\Asset\AssetInterface|null Asset instance when found, null when not.
* @return \Assetic\Contracts\Asset\AssetInterface|null Asset instance when found, null when not.
Copy link
Contributor

@Ocramius Ocramius Dec 16, 2020

Choose a reason for hiding this comment

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

Important: while it is indeed a third-party dependency that changes, this calls for a major version update.

Copy link
Contributor Author

@thomasvargiu thomasvargiu Dec 17, 2020

Choose a reason for hiding this comment

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

I agree, as I mentioned before, this is a BC break. Also, some functionalities are removed because not available in the new assetic library.

src/AssetManager/Service/AssetFilterManager.php
$filterClass = $filter;

if (!is_subclass_of($filterClass, 'Assetic\Filter\FilterInterface', true)) {
if (!is_subclass_of($filterClass, 'Assetic\Contracts\Filter\FilterInterface', true)) {
Copy link
Contributor

@Ocramius Ocramius Dec 16, 2020

Choose a reason for hiding this comment

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

Similar to other code changes that moved symbols, this is also a BC break

@RWOverdijk RWOverdijk merged commit a633e27 into RWOverdijk:master Dec 17, 2020
@RWOverdijk RWOverdijk mentioned this pull request Dec 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Ocramius Ocramius Ocramius approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thomasvargiu @jlaudenbach @RWOverdijk @Ocramius