pimcore icon indicating copy to clipboard operation
pimcore copied to clipboard

Impovement #17696 Upgrade dependencies where possible without BC break

Open michalananapps opened this issue 1 year ago • 9 comments

Changes in this pull request

Resolves #17696

Additional info

The file models/Document/Editable/Date.php requires a temporary workaround due to Carbon deprecating the formatLocalized method. Once support for the outputFormat option is dropped, this workaround will no longer be necessary.

The entries in the composer.json file under the require-dev section have been sorted. Additionally, phpstan/phpstan has been upgraded to version 1.12.6.

michalananapps avatar Oct 09 '24 07:10 michalananapps

Review Checklist

  • [ ] Target branch (11.4 for bug fixes, others 11.x)
  • [ ] Tests (if it's testable code, there should be a test for it - get help)
  • [ ] Docs (every functionality needs to be documented, see here)
  • [ ] Migration incl. install.sql (e.g. if the database schema changes, ...)
  • [ ] Upgrade notes (deprecations, important information, migration hints, ...)
  • [ ] Label
  • [ ] Milestone

github-actions[bot] avatar Oct 09 '24 07:10 github-actions[bot]

@fashxp The Codeception tests are failing for PHP 8.1 because the scheb/2fa-bundle requires PHP 8.2 or higher. In this case, should we downgrade the version of scheb/2fa-bundle to 6.x, or rather drop support for PHP 8.1?

michalananapps avatar Oct 09 '24 09:10 michalananapps

Is it not possible to support scheb/2fa-bundle v6 and v7?

I think this should be done for all libs, if possible, to ease upgrades for Pimcore users.

jdreesen avatar Oct 09 '24 09:10 jdreesen

Is it not possible to support scheb/2fa-bundle v6 and v7?

I think this should be done for all libs, if possible, to ease upgrades for Pimcore users.

This is possible as long as both versions are compatible in the areas used by Pimcore and its extensions. As for scheb/2fa-bundle, both versions should be fine as long as no one is using scheb_two_factor.google.window. https://github.com/scheb/2fa/blob/7.x/UPGRADE.md#6x-to-7x

michalananapps avatar Oct 09 '24 09:10 michalananapps

If you use a library in your project, you should add it as a dependency in the composer.json of your project to have control over its update.

This allows you to determine when the next version will be used and adapt the project code accordingly beforehand.

jdreesen avatar Oct 09 '24 10:10 jdreesen

If you use a library in your project, you should add it as a dependency in the composer.json of your project to have control over its update.

This allows you to determine when the next version will be used and adapt the project code accordingly beforehand.

You’re right. However, in the case of libraries used in Pimcore's core, you need to consider the core's compatibility with both versions. For example, assuming that Pimcore is currently using scheb/2fa-bundle version 5, when updating from 5.x to 6.x and declaring both versions as compatible in composer, it might be necessary to rewrite part of Pimcore's code responsible for authentication and maintain compatibility with both solutions (due to significant changes between 5.x and 6.x). In the case of comparing 6.x and 7.x (2fa-bundle), the differences are not as drastic, which in my opinion allows for both versions to be used.

michalananapps avatar Oct 09 '24 11:10 michalananapps

Sure, Pimcore needs to take care of supporting both version if both are allowed.

jdreesen avatar Oct 09 '24 11:10 jdreesen

Another thing. endroid/qr-code requirement should be moved to pimcore/admin-ui-classic-bundle. It is not used anywhere else.

michalananapps avatar Oct 10 '24 21:10 michalananapps

@michalananapps I just did a test installation and created a class with a date range. On saving I encounter the following problem which seems to be related to the nesbot/carbon update. That is a BC-Break since there is no available solution in myclabs/deep-copy since the latest version is 1.12. and I tested it with that version.


Timestamp: Wed Nov 06 2024 10:48:58 GMT+0100 (Central European Standard Time)
Status: 500 | Internal Server Error
URL: /admin/object/save?task=version
Method: PUT
Message: Cannot modify readonly property DatePeriod::$start
Trace: 
in /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php:267
#0 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(267): ReflectionProperty->setValue(Object(Carbon\CarbonPeriod), Object(Carbon\Carbon))
#1 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(214): DeepCopy\DeepCopy->copyObjectProperty(Object(Carbon\CarbonPeriod), Object(ReflectionProperty))
#2 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(150): DeepCopy\DeepCopy->copyObject(Object(Carbon\CarbonPeriod))
#3 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(267): DeepCopy\DeepCopy->recursiveCopy(Object(Carbon\CarbonPeriod))
#4 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(214): DeepCopy\DeepCopy->copyObjectProperty(Object(Pimcore\Model\DataObject\Testclass), Object(ReflectionProperty))
#5 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(150): DeepCopy\DeepCopy->copyObject(Object(Pimcore\Model\DataObject\Testclass))
#6 /var/www/html/vendor/myclabs/deep-copy/src/DeepCopy/DeepCopy.php(95): DeepCopy\DeepCopy->recursiveCopy(Object(Pimcore\Model\DataObject\Testclass))
#7 /var/www/html/dev/pimcore/pimcore/models/Version.php(217): DeepCopy\DeepCopy->copy(Object(Pimcore\Model\DataObject\Testclass))
#8 /var/www/html/dev/pimcore/pimcore/models/Version.php(156): Pimcore\Model\Version->marshalData(Object(Pimcore\Model\DataObject\Testclass))
#9 /var/www/html/dev/pimcore/pimcore/models/Element/AbstractElement.php(607): Pimcore\Model\Version->save()
#10 /var/www/html/dev/pimcore/pimcore/models/DataObject/Concrete.php(268): Pimcore\Model\Element\AbstractElement->doSaveVersion(NULL, false, true, false)
#11 /var/www/html/dev/pimcore/pimcore/models/DataObject/Concrete.php(206): Pimcore\Model\DataObject\Concrete->saveVersion(false, false, NULL)
#12 /var/www/html/dev/pimcore/pimcore/models/DataObject/AbstractObject.php(552): Pimcore\Model\DataObject\Concrete->update(true, Array)
#13 /var/www/html/dev/pimcore/pimcore/models/DataObject/Concrete.php(637): Pimcore\Model\DataObject\AbstractObject->save(Array)
#14 /var/www/html/vendor/pimcore/admin-ui-classic-bundle/src/Controller/Admin/DataObject/DataObjectController.php(1428): Pimcore\Model\DataObject\Concrete->save()
#15 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(181): Pimcore\Bundle\AdminBundle\Controller\Admin\DataObject\DataObjectController->saveAction(Object(Symfony\Component\HttpFoundation\Request))
#16 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#17 /var/www/html/vendor/symfony/http-kernel/Kernel.php(197): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#18 /var/www/html/vendor/symfony/runtime/Runner/Symfony/HttpKernelRunner.php(35): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#19 /var/www/html/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\HttpKernelRunner->run()
#20 /var/www/html/public/index.php(19): require_once('/var/www/html/v...')
#21 {main}

mattamon avatar Nov 06 '24 09:11 mattamon

@michalananapps something went wrong with the merge. There are a lot of commits that have nothing to do with the actual PR.

Regarding to your latest commit, our minimum Version of PHP is 8.1

8.2.0 The [DatePeriod::INCLUDE_END_DATE](https://www.php.net/manual/en/class.dateperiod.php#dateperiod.constants.include-end-date) constant and include_end_date property have been added.

So we cannot use DatePeriod since the tests are failing.

mattamon avatar Nov 06 '24 14:11 mattamon

@mattamon Sorry. I've made mistake with last rebase. I've created new PR on clean branch: https://github.com/pimcore/pimcore/pull/17799

In second branch I've also fixed PHP 8.1 compatibility.

michalananapps avatar Nov 06 '24 15:11 michalananapps

Closing in favor of https://github.com/pimcore/pimcore/pull/17799

mattamon avatar Nov 07 '24 06:11 mattamon