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

Make explicit some missing deps #415

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 24, 2021

Subject

Make explicit some missing deps.

I am targeting this branch, because these changes respect BC.

Changelog

### Fixed
- Missing dependencies required by this package.

@phansys phansys marked this pull request as ready for review July 24, 2021 15:12
@phansys phansys requested a review from a team July 24, 2021 15:12
VincentLanglet
VincentLanglet previously approved these changes Jul 24, 2021
composer.json Outdated
@@ -26,7 +30,7 @@
"gedmo/doctrine-extensions": "^3.0",
"matthiasnoback/symfony-dependency-injection-test": "^4.2.1",
"symfony/cache": "^4.4 || ^5.2",
"symfony/framework-bundle": "^4.4 || ^5.2",
Copy link
Member

Choose a reason for hiding this comment

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

Why this removal ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Class Controller was removed in symfony/framework-bundle:5.x:

use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
/**
* Controller for listing auditing information.
*
* @author Tim Nagel <[email protected]>
*
* @deprecated since sonata-project/entity-audit-bundle 1.1, will be remove in 2.0.
*
* NEXT_MAJOR: remove this controller
*/
class AuditController extends Controller

@phansys phansys requested review from VincentLanglet and a team July 24, 2021 23:14
VincentLanglet
VincentLanglet previously approved these changes Jul 24, 2021
@phansys phansys requested a review from a team July 25, 2021 02:17
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@phansys
Copy link
Member Author

phansys commented Oct 5, 2021

As stated in the checks for this PR, this package is not compatible with symfony/symfony:"5.3.*" and symfony/symfony:"6.0.*".
IMO, we should try to provide forward compatible bridges or mark these builds as optional.
WDYT?

@jordisala1991
Copy link
Member

As stated in the checks for this PR, this package is not compatible with symfony/symfony:"5.3.*" and symfony/symfony:"6.0.*". IMO, we should try to provide forward compatible bridges or mark these builds as optional. WDYT?

I would remove symfony/framework as a dev dependency and instead add it as an extra build (with the variant thing). And ignore the errors of static analysis tools. the class is deprecated and is not suposed to be used or modified anymore, so not a big deal if it is ignored by the tools. wdyt?

"symfony/config": "^4.4 || ^5.3 || ^6.0",
"symfony/http-kernel": "^4.4 || ^5.3 || ^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

why this as require dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this package is a Symfony bundle:

namespace SimpleThings\EntityAudit;
use Symfony\Component\HttpKernel\Bundle\Bundle;
class SimpleThingsEntityAuditBundle extends Bundle
{
}

Copy link
Member

Choose a reason for hiding this comment

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

Then dependency injection should be moved to, right?

@phansys
Copy link
Member Author

phansys commented Oct 6, 2021

I would remove symfony/framework as a dev dependency and instead add it as an extra build (with the variant thing). And ignore the errors of static analysis tools. the class is deprecated and is not suposed to be used or modified anymore, so not a big deal if it is ignored by the tools. wdyt?

I agree.

jordisala1991
jordisala1991 previously approved these changes Oct 24, 2021
Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

We need the dev-kit PR to add the variant thing, and IMO we should move dependency injection to require section. Other than that looks great

- php-version: '8.0'
dependencies: highest
allowed-to-fail: false
variant: symfony/framework-bundle
Copy link
Member

Choose a reason for hiding this comment

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

imo we should make explicit the 4.4 version

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

VincentLanglet
VincentLanglet previously approved these changes Oct 24, 2021
@VincentLanglet VincentLanglet merged commit e546f7f into sonata-project:1.x Oct 25, 2021
@VincentLanglet
Copy link
Member

Thanks @phansys

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

Successfully merging this pull request may close these issues.

5 participants