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

Prepare major release #609

Merged
merged 7 commits into from
Oct 8, 2021
Merged

Prepare major release #609

merged 7 commits into from
Oct 8, 2021

Conversation

core23
Copy link
Member

@core23 core23 commented Oct 7, 2021

Subject

I am targeting this branch, because this should be the last changes for the next major release.

Changelog

### Changed
- Made `sonata.seo.sitemap.manager` service private

### Removed
- Remove all `sonata.seo.*.class` parameters from container

@core23 core23 added the major label Oct 7, 2021
@core23 core23 force-pushed the prepare-major branch 2 times, most recently from 0aa95b4 to 0af359f Compare October 7, 2021 20:46
@core23 core23 requested a review from a team October 7, 2021 20:47
@VincentLanglet
Copy link
Member

If we follow the same strategy than other bundle, we should also

And we might determine what we do with #600 (review)

@core23 core23 force-pushed the prepare-major branch 2 times, most recently from 596d228 to 9929d6a Compare October 7, 2021 20:51
@core23
Copy link
Member Author

core23 commented Oct 7, 2021

Done

And we might determine what we do with #600 (review)

We can still implement the feature on the next major version

@core23 core23 force-pushed the prepare-major branch 2 times, most recently from 5e0c2d0 to 1aa5c38 Compare October 7, 2021 21:13
src/Resources/config/services.php Outdated Show resolved Hide resolved
$containerConfigurator->services()

->set('sonata.seo.page.default', '%sonata.seo.page.default.class%')
->set('sonata.seo.page.default', SeoPage::class)
->public()
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the usages, but do we need services to be public?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if symfony still recommends using the service container. If it's deprecated we could make this private.

Copy link
Member

@franmomu franmomu Oct 8, 2021

Choose a reason for hiding this comment

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

Looks like it depends on the usage:

In addition, services not meant to be used by the application directly, should be defined as private. For public services, aliases should be created from the interface/class to the service id.

(I haven checked, but I guess they don't recommend using the service container directly.)

src/Sitemap/SourceManager.php Outdated Show resolved Hide resolved
@core23 core23 force-pushed the prepare-major branch 2 times, most recently from 74f9b04 to 8d5a296 Compare October 8, 2021 06:02
Comment on lines -78 to +81
$connectionIterator = new Definition('%sonata.seo.exporter.database_source_iterator.class%', [
$connectionIterator = new Definition(DoctrineDBALConnectionSourceIterator::class, [
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with this, but if we have paramteter here should not be better to keep it and set it in config?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could improve this by allowing a custom service via the config, but for now we should get rid of all class parameters.

@@ -86,7 +89,7 @@ private function configureSitemap(array $config, ContainerBuilder $container): v
// define the sitemap proxy iterator
$sitemapIteratorId = 'sonata.seo.source.doctrine_sitemap_iterator_'.$pos;

$sitemapIterator = new Definition('%sonata.seo.exporter.sitemap_source_iterator.class%', [
$sitemapIterator = new Definition(SymfonySitemapSourceIterator::class, [
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with this, but if we have paramteter here should not be better to keep it and set it in config?

@wbloszyk
Copy link
Member

wbloszyk commented Oct 8, 2021

Before next major release we should decide about one importan thing.

After 2 months (59 days) support for PHP 7.3 will be end. If we drop it now then we can also declare types for parameters. Do it in stable branch will be BC.

WDYT? @sonata-project/contributors

@franmomu
Copy link
Member

franmomu commented Oct 8, 2021

Before next major release we should decide about one importan thing.

After 2 months (59 days) support for PHP 7.3 will be end. If we drop it now then we can also declare types for parameters. Do it in stable branch will be BC.

WDYT? @sonata-project/contributors

It's already dropped in this bundle:

"php": "^7.4 || ^8.0",

@wbloszyk
Copy link
Member

wbloszyk commented Oct 8, 2021

I want check it this bundle before major release in the evening. So can we wait with major release to tomorrow?

@core23 core23 merged commit 9be74b1 into sonata-project:3.x Oct 8, 2021
@core23 core23 deleted the prepare-major branch October 8, 2021 14:41
@VincentLanglet
Copy link
Member

I want check it this bundle before major release in the evening. So can we wait with major release to tomorrow?

We should try with an alpha/beta version anyway

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