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

Remove interfaces to improve flexibility for future changes #57

Merged
merged 2 commits into from
Jul 29, 2019

Conversation

ausi
Copy link
Member

@ausi ausi commented Jul 28, 2019

I think creating an interface for every class in this library was a mistake. Every feature we added to the 1.0 beta version broke BC because interfaces had to be changed.

IMO we should keep the following interfaces:

  • ImageInterface
  • DeferredImageInterface
  • ResizerInterface
  • DeferredResizerInterface
  • DeferredImageStorageInterface
  • PictureInterface
  • PictureGeneratorInterface

and remove these:

  • ImageDimensionsInterface
  • ImportantPartInterface
  • PictureConfigurationInterface
  • PictureConfigurationItemInterface
  • ResizeCalculatorInterface
  • ResizeConfigurationInterface
  • ResizeCoordinatesInterface
  • ResizeOptionsInterface

This would enable us to add more options and configurations in minor releases.

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

I guess we should completely remove the interfaces then and not deprecate them.

@leofeyer
Copy link
Member

+1 for removing the interfaces in v1.0.

@ausi
Copy link
Member Author

ausi commented Jul 28, 2019

I guess we should completely remove the interfaces then and not deprecate them.

I generally agree. I removed all of them except the ResizeConfigurationInterface::MODE_* constants because IMO it’s very likely that they get used.

@Toflar
Copy link
Member

Toflar commented Jul 29, 2019

I don't quite understand that point of view. Sure, they might have been used - just like any other interface. That's the purpose of alpha versions (0.* versions), you don't have to guarantee BC so when you feel like you're ready to release the first stable version (1.0.0), you can release a clean library. So I don't see why we would need to distinguish here. Either break BC completely or not at all.

@leofeyer
Copy link
Member

Fair point.

@ausi
Copy link
Member Author

ausi commented Jul 29, 2019

I think that the ResizeConfigurationInterface::MODE_* constants are special in this case. And because it is very easy to keep them (and without overhead because the interface file never gets loaded if not used) I don’t see a reason to break BC here.

@Toflar
Copy link
Member

Toflar commented Jul 29, 2019

Personally, I would clean them up but as you're the one maintaining stuff, I'll leave it up to you.

@ausi ausi force-pushed the feature/remove-interfaces branch from 64ff676 to 9dcfc20 Compare July 29, 2019 17:50
@ausi ausi force-pushed the feature/remove-interfaces branch from 9dcfc20 to 8266458 Compare July 29, 2019 18:08
@ausi ausi merged commit 1846080 into contao:master Jul 29, 2019
@ausi ausi deleted the feature/remove-interfaces branch July 29, 2019 18:13
leofeyer pushed a commit to contao/contao that referenced this pull request Jul 30, 2019
Description
-----------

Roadmap:

1. [x] Get agreement on contao/image#57 and merge it
2. [x] Release `contao/image` 1.0.0-beta4 on Monday evening
3. [ ] Merge this PR before releasing RC2

Commits
-------

84d8d14 Remove contao/image interfaces
b29276d Use 1.0.0-beta4 version of contao/image
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Jul 30, 2019
Description
-----------

Roadmap:

1. [x] Get agreement on contao/image#57 and merge it
2. [x] Release `contao/image` 1.0.0-beta4 on Monday evening
3. [ ] Merge this PR before releasing RC2

Commits
-------

84d8d146 Remove contao/image interfaces
b29276db Use 1.0.0-beta4 version of contao/image
@BugBuster1701
Copy link

When updating a 4.8.RC1 installation:

  • Updating contao/image (1.0.0-beta3 => 1.0.0-beta4)

PHP Warning: Declaration of Contao\CoreBundle\Image\LegacyResizer::executeResize(Contao\Image\ImageInterface $image, Contao\Image\ResizeCoordinatesInterface $coordinates, string $path, Contao\Image\ResizeOptionsInterface $options): Contao\Image\ImageInterface should be compatible with Contao\Image\DeferredResizer::executeResize(Contao\Image\ImageInterface $image, Contao\Image\ResizeCoordinates $coordinates, string $path, Contao\Image\ResizeOptions $options): Contao\Image\ImageInterface in vendor/contao/core-bundle/src/Image/LegacyResizer.php on line 31

PHP Warning: Uncaught ReflectionException: Class Contao\Image\ResizeCoordinatesInterface not found in vendor/contao/core-bundle/src/Image/LegacyResizer.php:31

PHP Fatal error: Declaration of Contao\CoreBundle\Image\LegacyResizer::resize(Contao\Image\ImageInterface $image, Contao\Image\ResizeConfigurationInterface $config, Contao\Image\ResizeOptionsInterface $options): Contao\Image\ImageInterface must be compatible with Contao\Image\Resizer::resize(Contao\Image\ImageInterface $image, Contao\Image\ResizeConfiguration $config, Contao\Image\ResizeOptions $options): Contao\Image\ImageInterface in vendor/contao/core-bundle/src/Image/LegacyResizer.php on line 31

Do I have to update to 4.8.RC2 now to make the installation work again?

@Toflar
Copy link
Member

Toflar commented Aug 4, 2019

That sounds like the PHP 7.2.20/7.3.7 issue.

@leofeyer
Copy link
Member

leofeyer commented Aug 4, 2019

Do I have to update to 4.8.RC2 now to make the installation work again?

Yes, you have. Contao 4.8.0-RC1 is not compatible with contao/image 1.0.0-beta4.

@ausi
Copy link
Member Author

ausi commented Aug 5, 2019

Do I have to update to 4.8.RC2 now to make the installation work again?

You could also add a conflict for 1.0.0-beta4 or require 1.0.0-beta3 explicitly, but updating to 4.8.0-RC2 is the way to go here I think.

@BugBuster1701
Copy link

"conflict" was my first step. Now i have make the update to RC2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants