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

Bumped version of the components and dropped old versions. #29

Open
wants to merge 15 commits into
base: 2.5.x
Choose a base branch
from

Conversation

tasselchof
Copy link

No description provided.

@@ -4,7 +4,7 @@

namespace Laminas\ApiTools\Doctrine\Admin\Model;

use Doctrine\Common\Persistence\ObjectManager;
use Doctrine\Persistence\ObjectManager;
Copy link
Member

Choose a reason for hiding this comment

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

Due to the change of type on protected $objectManager, this becomes a BC break requiring a major release 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Same applies to other properties

@@ -4,7 +4,7 @@

namespace Laminas\ApiTools\Doctrine\Server\Event;

use Doctrine\Common\Persistence\ObjectManager;
use Doctrine\Persistence\ObjectManager;
Copy link
Member

Choose a reason for hiding this comment

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

API change is more stark here: parameter type changed

composer.json Outdated
@@ -32,16 +35,16 @@
"php": "^7.3 || ~8.0.0",
"laminas-api-tools/api-tools": "^1.3",
"laminas-api-tools/api-tools-rest": "^1.3.2",
"laminas/laminas-hydrator": "^2.4.2 || ^3.0",
"laminas/laminas-hydrator": "^2.4.2 || ^3.0 || ^4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop ^2 here - keeping only two majors, which is sufficient for a migration

composer.json Outdated
"doctrine/doctrine-mongo-odm-module": "^0.11.0 || ^1.0.0 || ^2.0.0",
"doctrine/doctrine-orm-module": "^1.1.8 || ^2.1.3",
"doctrine/doctrine-orm-module": "^4.0 || ^5.1",
Copy link
Member

Choose a reason for hiding this comment

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

We should test only ^5.1, and declare that <5.1 is in conflict: (because we no longer test it)

composer.json Outdated
},
"require-dev": {
"doctrine/doctrine-module": "^1.2.0 || ^2.1.7",
"doctrine/doctrine-module": "^4.0 || ^5.1",
Copy link
Member

Choose a reason for hiding this comment

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

We should test only ^5.1, and add <5.1 to conflict:

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius I didn't dropped 4.0 because we have this dependency:

https://github.com/API-Skeletons/doctrine-orm-hydration-module/blob/master/composer.json#L28

And this external module won't allow us to use only 5.1 version.

Copy link
Member

Choose a reason for hiding this comment

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

So we aren't testing ^5.1? Then we shouldn't include ^5.1 in our supported dependency range.

Copy link
Author

@tasselchof tasselchof Mar 15, 2022

Choose a reason for hiding this comment

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

Maybe I should do fix there? I forked this module and it works well with ^5.1. The problem is that to do this I forked 3 modules, that have a dependencies, this is why I committed here ^5.1. I think it makes sense to make PR there and after to finish this one. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so here's what is not clear to me:

  • is ^4.0 required by laminas-api-tools/api-tools-doctrine? If not, it should be dropped
  • if ^5.1 works, we should use that, and only that, and drop all older versions. Introducing support for older versions should be discussed in a separate patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tasselchof API Skeletons is my org. If you need an update on the hydration module in support of your changes here, that can be arranged.

Copy link
Author

@tasselchof tasselchof Mar 15, 2022

Choose a reason for hiding this comment

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

@TomHAnderson hi! I already made PR to add this module, locally tests are passing: API-Skeletons/doctrine-orm-hydration-module#4. If you merge it I can resume here.

Copy link
Author

@tasselchof tasselchof Mar 17, 2022

Choose a reason for hiding this comment

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

@Ocramius what is the best way further? Now it works fine with ORM, but here is the deal:

 Problem 1
    - doctrine/doctrine-mongo-odm-module[0.11.0, 1.0.0] require doctrine/doctrine-module ^1.2 -> found doctrine/doctrine-module[1.2.0] but it conflicts with your root composer.json require (^5.1).
    - doctrine/doctrine-mongo-odm-module 1.1.0 requires doctrine/doctrine-module ^1.2 || 2.1.7 -> found doctrine/doctrine-module[1.2.0, 2.1.7] but it conflicts with your root composer.json require (^5.1).
    - doctrine/doctrine-mongo-odm-module 1.1.1 requires doctrine/doctrine-module ^1.2 || ^2.1.7 -> found doctrine/doctrine-module[1.2.0, 2.1.7, 2.1.8, 2.1.9, 2.1.10] but it conflicts with your root composer.json require (^5.1).
    - doctrine/doctrine-mongo-odm-module[2.0.0, ..., 2.0.2] require doctrine/doctrine-module ^4.0 -> found doctrine/doctrine-module[4.0.0, ..., 4.4.1] but it conflicts with your root composer.json require (^5.1).
    - Root composer.json requires doctrine/doctrine-mongo-odm-module ^0.11.0 || ^1.0.0 || ^2.0.0 -> satisfiable by doctrine/doctrine-mongo-odm-module[0.11.0, 1.0.0, 1.1.0, 1.1.1, 2.0.0, 2.0.1, 2.0.2].

And I never used ODM module and we don't have hydration module as phpro/zf-doctrine-hydration-module is abandoned and as I know API-Skeletons/doctrine-orm-hydration-module does not support odm.

Copy link
Author

Choose a reason for hiding this comment

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

@TomHAnderson hydration module is tested on php 8.0?

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius Ocramius added the Enhancement New feature or request label Mar 15, 2022
@Ocramius
Copy link
Member

PHP 7.3 should probably be dropped from our supported versions

@javabudd
Copy link

This PR is greatly appreciated and badly needed!

@tasselchof
Copy link
Author

tasselchof commented Apr 19, 2022

The problem with that I don't know what to do with doctrine/doctrine-mongo-odm-module - this is the dependency that is preventing from update now, if @Ocramius can tell what to do, I can finish it.

@Ocramius
Copy link
Member

Not sure how to proceed either: practically, supporting version ranges that are not tested by our CI is not feasible, as it's just a lie towards end-users.

Only version ranges touched by CI should be part of our dependency range.

If doctrine/doctrine-mongo-odm-module is a problem, perhaps we should really just yeet it out.

@tasselchof
Copy link
Author

Not sure how to proceed either: practically, supporting version ranges that are not tested by our CI is not feasible, as it's just a lie towards end-users.

Only version ranges touched by CI should be part of our dependency range.

If doctrine/doctrine-mongo-odm-module is a problem, perhaps we should really just yeet it out.

As I see it it is, I will do not PR without it and we will see how tests will go through. Maybe we can drop support for ODM for now? If someone will eventually update doctrine/doctrine-mongo-odm-module it will come back in a future.

@Ocramius
Copy link
Member

Maybe we can drop support for ODM for now?

Would require a major version, if any of our API is affected.

@tasselchof
Copy link
Author

Would require a major version, if any of our API is affected.

Should I do something except updating this PR to make it happen?

@Ocramius
Copy link
Member

Just push to this branch: practically, all we want is to try bumping dependencies up, so we can try until we get to a dependency range that is fully tested by CI.

@tasselchof
Copy link
Author

Okey, I'll try to do it and we will see =)

@tasselchof
Copy link
Author

@Ocramius I had to fix few things in tests, as newer versions of doctrine libraries does not have some models, that were used. Please, write me if I have to do something else. I think tests should be passing now.

@javabudd
Copy link

javabudd commented Apr 20, 2022

Not sure how to proceed either: practically, supporting version ranges that are not tested by our CI is not feasible, as it's just a lie towards end-users.

Only version ranges touched by CI should be part of our dependency range.

If doctrine/doctrine-mongo-odm-module is a problem, perhaps we should really just yeet it out.

I would vote towards yeeting it out. There are several other ODM providers and I think a module solely for mongoDB is the wrong approach. If we remove support on the next major version, a more generic ODM adapter could be introduced into the major version as a patch/minor update.

@javabudd
Copy link

An example of a more generic adapter would be how the the doctrine/dbal library handles drivers (https://github.com/doctrine/dbal/tree/3.3.x/src/Driver)

@tasselchof
Copy link
Author

@javabudd I did it, just waiting for @Ocramius to run tests.

@Ocramius
Copy link
Member

You should get a CI run on your fork too, BTW

@Ocramius
Copy link
Member

Lots to be checked here :D

@javabudd
Copy link

@tasselchof Do you need help getting this across the finish line? I could help out with the requested changes if needed.

@tasselchof
Copy link
Author

@javabudd changes are done, just @Ocramius wrote, that there are a lot to be reviewed here - so we wait for review now.

Copy link
Member

@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.

TLDR:

  • some CS fixes require moving to psr/container + laminas/laminas-servicemanager:^3.11.2
  • try vendor/bin/phpcbf
  • some psalm errors seem to indicate that we should drop MongoDB ODM support completely
  • we need a roave/backward-compatibility-check report
  • we need a maglnet/composer-require-checker report
  • we may need to do a hard cut and bump major version here, as a reasonable direction forward

Code Style

Error: Expected class name interop\container\containerinterface; found Interop\Container\ContainerInterface

These just need to switch over to psr/container interfaces - that fixes the problem. For that to work, we need to make sure that we're adding laminas/laminas-servicemanager:^3.11.2 to the dependencies (sadly, but that reduces the risk of perceived BC breaks downstream).

The rest of the CS issues seem to be fixable via vendor/bin/phpcbf

Static Analysis

The psalm issues are much deeper and much more problematic: I didn't expect the MongoDB ODM module to be this engrained in this system, and it makes the whole module extremely fragile, especially as it's require-dev only.

I feel like we should deprecate all ODM classes, and add any failures reported on them to our psalm baseline perhaps 🤔

I'd also be fine with just calling this a breakage, removing all ODM-related classes, and then bumping major version, reducing long-term maintenance complexity, which is really exploding in this complex integration layer.

BC Checking

I would also need a report from roave/backward-compatibility-check comparing 2.5.x to this HEAD, in order to figure out the hidden BC breaks caused by API changes.

composer.json Outdated Show resolved Hide resolved
src/Server/Event/DoctrineResourceEvent.php Outdated Show resolved Hide resolved
src/Server/Event/DoctrineResourceEvent.php Outdated Show resolved Hide resolved
src/Server/Event/Listener/CollectionListener.php Outdated Show resolved Hide resolved
src/Server/Resource/DoctrineResource.php Show resolved Hide resolved
use function json_decode;
use function json_encode;

class CRUDTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we're starting to drop tests in order to keep things alive is a strong indicator that we should declare the BC break explicitly and move on here 🤔

Copy link
Author

Choose a reason for hiding this comment

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

We should, as dropping ODM support is a BC break.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but it's really for the best: ODM users are a drop in the ocean, and the complexity deriving from supporting both is mind-boggling.

To this day, I've yet to see a single software system where MongoDB is an architectural requirement for operation, so dropping support is a stance that I'm fully backing, and can defend long-term.

test/src/Server/Validator/NoObjectExistsFactoryTest.php Outdated Show resolved Hide resolved
test/src/Server/Validator/ObjectExistsFactoryTest.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated
"doctrine/orm": "<2.6.3"
"doctrine/orm": "<2.6.3",
"doctrine/doctrine-module": "<4.0",
"doctrine/doctrine-orm-module": "<4.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can we run maglnet/composer-require-checker on the codebase, to discover if any of these conflicts are part of symbols that we use?

If they are, we should just have them in require instead.

Copy link
Author

Choose a reason for hiding this comment

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

php composer-require-checker.phar check composer.json
ComposerRequireChecker 4.0.0@baa11a4e9e5072117e3d180ef16c07036cafa4a2
The following 51 unknown symbols were found:
+-----------------------------------------------------------+--------------------+
| Unknown Symbol                                            | Guessed Dependency |
+-----------------------------------------------------------+--------------------+
| DoctrineModule\Persistence\ObjectManagerAwareInterface    |                    |
| DoctrineModule\Validator\NoObjectExists                   |                    |
| DoctrineModule\Validator\ObjectExists                     |                    |
| Doctrine\Common\Collections\ArrayCollection               |                    |
| Doctrine\Laminas\Hydrator\DoctrineObject                  |                    |
| Doctrine\ORM\EntityManager                                |                    |
| Doctrine\ORM\EntityManagerInterface                       |                    |
| Doctrine\ORM\NoResultException                            |                    |
| Doctrine\ORM\QueryBuilder                                 |                    |
| Doctrine\ORM\Tools\Pagination\Paginator                   |                    |
| Doctrine\Persistence\ObjectManager                        |                    |
| interop\container\containerinterface                      |                    |
| Laminas\ApiTools\Admin\Exception\InvalidArgumentException |                    |
| Laminas\ApiTools\Admin\Exception\RuntimeException         |                    |
| Laminas\ApiTools\Admin\Model\AbstractAutodiscoveryModel   |                    |
| Laminas\ApiTools\Admin\Model\DocumentationModel           |                    |
| Laminas\ApiTools\Admin\Model\InputFilterModel             |                    |
| Laminas\ApiTools\Admin\Model\ModuleEntity                 |                    |
| Laminas\ApiTools\Admin\Model\ModuleModel                  |                    |
| Laminas\ApiTools\Admin\Model\ModulePathSpec               |                    |
| Laminas\ApiTools\Admin\Model\NewRestServiceEntity         |                    |
| Laminas\ApiTools\Admin\Model\RestServiceEntity            |                    |
| Laminas\ApiTools\Admin\Model\RestServiceModelFactory      |                    |
| Laminas\ApiTools\Admin\Model\RestServiceResource          |                    |
| Laminas\ApiTools\Admin\Model\RpcServiceEntity             |                    |
| Laminas\ApiTools\Admin\Model\RpcServiceModelFactory       |                    |
| Laminas\ApiTools\Admin\Model\RpcServiceResource           |                    |
| Laminas\ApiTools\Admin\Utility                            |                    |
| Laminas\ApiTools\ApiProblem\ApiProblem                    |                    |
| Laminas\ApiTools\Configuration\ConfigResource             |                    |
| Laminas\ApiTools\Configuration\ConfigResourceFactory      |                    |
| Laminas\ApiTools\ContentNegotiation\ViewModel             |                    |
| Laminas\ApiTools\Doctrine\Admin\Model\InvokableFactory    |                    |
| Laminas\EventManager\Event                                |                    |
| Laminas\EventManager\EventManager                         |                    |
| Laminas\EventManager\EventManagerAwareInterface           |                    |
| Laminas\EventManager\EventManagerInterface                |                    |
| Laminas\EventManager\ListenerAggregateInterface           |                    |
| Laminas\EventManager\SharedEventManager                   |                    |
| Laminas\EventManager\SharedEventManagerInterface          |                    |
| Laminas\Filter\FilterChain                                |                    |
| Laminas\Filter\StringToLower                              |                    |
| Laminas\Filter\Word\CamelCaseToDash                       |                    |
| Laminas\InputFilter\CollectionInputFilter                 |                    |
| Laminas\InputFilter\InputFilterInterface                  |                    |
| Laminas\ModuleManager\ModuleManager                       |                    |
| Laminas\Mvc\Controller\AbstractActionController           |                    |
| Laminas\Mvc\Controller\ControllerManager                  |                    |
| Laminas\Mvc\ModuleRouteListener                           |                    |
| Laminas\Mvc\Service\AbstractPluginManagerFactory          |                    |
| Laminas\Paginator\Adapter\AdapterInterface                |                    |
+-----------------------------------------------------------+--------------------+

Copy link
Member

Choose a reason for hiding this comment

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

That is a lot of stuff @_@

I don't think we can add all these in this patch though...

Copy link
Member

Choose a reason for hiding this comment

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

I think we can fix these here by just adding the require/ blocks in the code, but I'd be OK for us doing this in 3.1.x later

@Ocramius
Copy link
Member

That report seems broken 😁

@tasselchof
Copy link
Author

Sorry about that =)

report.txt

@javabudd
Copy link

@tasselchof This one is so close to the finish line! I don't mind helping with the rest of the changes if you need.

@tasselchof
Copy link
Author

@tasselchof This one is so close to the finish line! I don't mind helping with the rest of the changes if you need.

I am all in to finish it myself, but it depends on @Ocramius and reviewers.

composer.json Outdated
"doctrine/orm": "<2.6.3"
"doctrine/orm": "<2.6.3",
"doctrine/doctrine-module": "<4.0",
"doctrine/doctrine-orm-module": "<4.0"
Copy link
Member

Choose a reason for hiding this comment

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

That is a lot of stuff @_@

I don't think we can add all these in this patch though...

@@ -1,6 +1,5 @@
<?xml version="1.0"?>
<psalm
totallyTyped="true"
Copy link
Member

Choose a reason for hiding this comment

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

level="1" should be put here as a replacement

@@ -4,7 +4,7 @@

namespace Laminas\ApiTools\Doctrine\Admin\Model;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

This needs reverting

@@ -4,14 +4,14 @@

namespace Laminas\ApiTools\Doctrine\Admin\Model;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

Needs reverting

* @return EventManagerInterface
*/
public function getEventManager()
{
if (! $this->events) {
if (empty($this->events)) {
Copy link
Member

Choose a reason for hiding this comment

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

We should really never ever ever use empty()

@@ -308,7 +302,7 @@ class_exists(DocumentManager::class)
* @param array $config
* @return array
*/
protected function loadConfiguredListeners(ContainerInterface $container, array $config)
protected function loadConfiguredListeners(containerinterface $container, array $config)
Copy link
Member

Choose a reason for hiding this comment

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

Casing to be reverted

@@ -4,7 +4,7 @@

namespace LaminasTest\ApiTools\Doctrine\Admin\Controller;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

Revert needed

@@ -4,7 +4,7 @@

namespace LaminasTest\ApiTools\Doctrine\Admin\Model;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

Revert needed

@@ -4,7 +4,7 @@

namespace LaminasTest\ApiTools\Doctrine\Admin\Model;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

Revert needed

@@ -4,7 +4,7 @@

namespace LaminasTest\ApiTools\Doctrine\Admin\Model;

use Interop\Container\ContainerInterface;
use interop\container\containerinterface;
Copy link
Member

Choose a reason for hiding this comment

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

Revert needed

@Ocramius
Copy link
Member

TBH, I would say that we:

  1. fix the broken bits I commented on
  2. merge this against 3.0.x (I'll do it after approval)

@javabudd
Copy link

javabudd commented Jul 29, 2022

@tasselchof I've applied the review changes on this PR targeting your 2.5.x branch: orderadmin#1

@tasselchof
Copy link
Author

@javabudd merged. @Ocramius what’s next? :)

@Ocramius Ocramius added this to the 2.5.0 milestone Jul 30, 2022
@Ocramius
Copy link
Member

It's all red? :P

* @return ObjectManager
*/
public function getObjectManager()
public function getObjectManager(): ObjectManager
Copy link
Member

Choose a reason for hiding this comment

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

add return type for public method is BC break for non-final class, ref https://3v4l.org/egUAq

* @return $this
*/
public function setObjectManager(ObjectManager $objectManager)
public function setObjectManager(ObjectManager $objectManager): DoctrineResourceEvent
Copy link
Member

Choose a reason for hiding this comment

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

here as well, BC break

@tasselchof
Copy link
Author

It's all red? :P

We need bump of one module for hydration from API-Skeletons: https://github.com/laminas-api-tools/api-tools-doctrine/runs/7590699927?check_suite_focus=true#step:4:160

@TomHAnderson would you be so kind to do it?

@tasselchof
Copy link
Author

@Ocramius is it ok to use my fork of this module? It works perfect and pull request there is taking too long time. I am speaking about https://github.com/API-Skeletons/doctrine-orm-hydration-module package.

@Ocramius
Copy link
Member

@tasselchof not in the official package (here), but you can certainly use your fork locally.

See https://naderman.de/slippy/src/?file=2012-11-22-You-Thought-Composer-Couldnt-Do-That.html#30

@tasselchof
Copy link
Author

@tasselchof not in the official package (here), but you can certainly use your fork locally.

See https://naderman.de/slippy/src/?file=2012-11-22-You-Thought-Composer-Couldnt-Do-That.html#30

Thanks for the reference - I am using it in our project though satis =)) Just I want to bump officials package.

@Ocramius
Copy link
Member

OSS requires lots of patience: you aren't just trying to fix coding bugs, but also fighting for time allocation with people's free time :-)

Some of my own packages aren't compatible with PHP 8.1 yet, for example.

@tasselchof
Copy link
Author

OSS requires lots of patience: you aren't just trying to fix coding bugs, but also fighting for time allocation with people's free time :-)

Some of my own packages aren't compatible with PHP 8.1 yet, for example.

Yeah, I understand this =)) Will be patient and contribute where I can. Thank you for your time, that you committing to this.

@javabudd
Copy link

OSS requires lots of patience: you aren't just trying to fix coding bugs, but also fighting for time allocation with people's free time :-)

Some of my own packages aren't compatible with PHP 8.1 yet, for example.

I totally get this reasoning, however, PHP 7.4 reaches EOL on the 28th of November. This is one of the few Laminas libraries that still has yet to support even PHP 8.0 and I think prioritizing this merge/version update is very important.

Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I'm around 25% through reviewing, and still only in the config files, but I'm seeing a lot of troubling changes. In particular, there's a lot of places where these are referring to Apigility classes where they should be referring to Laminas API Tools classes. This is a dangerous regression, as it (a) undoes existing work to migrate the code to the new namespaces, and (b) makes the code incompatible with userland code which was migrated to Laminas already.

On top of that, there are a ton of CI/CD warnings, most of which can easily be fixed by running phpcbf, or by adding annotations to suppress the warnings (particularly in the case of long lines). Addressing these will simplify the job of those of us reviewing.

Finally, due to the larger changes in terms of dependency upgrades/changes, there's no way this can target a new minor, as these changes would cause breakage in any application that extends one of these classes (e.g., the DoctrineResourceEvent, where property types and return types change or are added). If we are to continue evaluating this, we'll need a 3.0.x branch, and will need to target this for 3.0.0. If you are still willing to work on the patch, let us know, and one of us will create the new branch for you.

composer.json Outdated
"laminas/laminas-stdlib": "^3.3",
"laminas/laminas-view": "^2.11.3",
"laminas/laminas-zendframework-bridge": "^1.0",
"phpro/zf-doctrine-hydration-module": "^2.0.1 || ^3.0 || ^4.1"
"laminas/laminas-zendframework-bridge": "^1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed anymore? Are any of the dependencies using ZF deps?

@@ -21,7 +19,7 @@
'options' => [
'route' => '/api-tools/api/module[/:name]/doctrine-rpc[/:controller_service_name]',
'defaults' => [
'controller' => Controller\DoctrineRpcService::class,
'controller' => 'ZF\Apigility\Doctrine\Admin\Controller\DoctrineRpcService',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change. We should be using the Laminas\ApiTools\Doctrine namespace, not the ZF\Apigility\Doctrine namespace.

This change would be a regression, reverting to pre-migration changes.

Revert on lines 42 and 52 as well, please.

Comment on lines +62 to +63
'ZF\Apigility\Doctrine\Admin\Model\DoctrineAutodiscoveryModel' => DoctrineAutodiscoveryModel::class,
'ZF\Apigility\Doctrine\Admin\Model\DoctrineMetadataServiceResource' => DoctrineMetadataServiceResource::class,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note the warnings from phpcs below. You can either (a) fix the issues, or (b) add annotations to suppress the warnings (the rule in question is General.Files.LineLength.TooLong).

Comment on lines +89 to +92
'ZF\Apigility\Doctrine\Admin\Controller\DoctrineAutodiscovery' => 'Json',
'ZF\Apigility\Doctrine\Admin\Controller\DoctrineRestService' => 'HalJson',
'ZF\Apigility\Doctrine\Admin\Controller\DoctrineRpcService' => 'HalJson',
'ZF\Apigility\Doctrine\Admin\Controller\DoctrineMetadataService' => 'HalJson',
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be reverted, please. We should only be referring to Laminas\ApiTools classes for these sorts of mappings at this point.

composer.json Outdated
"php": "^7.3 || ~8.0.0",
"php": "^7.4 || ~8.0.0",
"ext-json": "*",
"api-skeletons/doctrine-orm-hydration-module": "^1.0",
Copy link

Choose a reason for hiding this comment

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

this should probably be replaced with doctrine/doctrine-laminas-hydrator

Copy link
Author

Choose a reason for hiding this comment

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

this should probably be replaced with doctrine/doctrine-laminas-hydrator

Probably, but it might be not so easy.

@tasselchof
Copy link
Author

What need to be fixed to merge it? I am using my fork in my projects, although I think somebody might need this one.

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

Successfully merging this pull request may close these issues.

6 participants