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.

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

Dependencies update with assetic framework #221

Merged
merged 5 commits into from
Dec 17, 2020
Merged

Dependencies update with assetic framework #221

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

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

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

I'll try to rebase it

@thomasvargiu
Copy link
Contributor Author

@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

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
@@ -9,7 +9,7 @@ interface ResolverInterface
*
* @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

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

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
@@ -138,7 +138,7 @@ protected function ensureByFilter(AssetInterface $asset, $filter)

$filterClass = $filter;

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

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
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius approved these changes

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