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 transitive dependencies #5784

Merged
merged 2 commits into from Aug 8, 2022

Conversation

yguedidi
Copy link
Contributor

@yguedidi yguedidi commented May 8, 2022

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documentation no
Translation no
CHANGELOG.md no
License MIT

Have the code rely on transitive dependencies (dependencies not explicitly required) is not good, as their versioning is not managed by us.

This PR remove relying on transitive dependencies by requiring them explicitly at their installed minor version from composer.lock.
Transitive dependencies are identified with ComposerRequireChecker

Here the result on current 2.6.0 branch (d510dc6):

The following 44 unknown symbols were found:
+--------------------------------------------------------+--------------------+
| Unknown Symbol                                         | Guessed Dependency |
+--------------------------------------------------------+--------------------+
| DAMA\DoctrineTestBundle\DAMADoctrineTestBundle         |                    |
| Datetime                                               |                    |
| Doctrine\Bundle\FixturesBundle\DoctrineFixturesBundle  |                    |
| Doctrine\Bundle\FixturesBundle\Fixture                 |                    |
| Doctrine\Common\Collections\ArrayCollection            |                    |
| Doctrine\Common\DataFixtures\DependentFixtureInterface |                    |
| Doctrine\Common\EventSubscriber                        |                    |
| Doctrine\Common\Persistence\ManagerRegistry            |                    |
| Doctrine\DBAL\Exception\DriverException                |                    |
| Doctrine\DBAL\Platforms\SqlitePlatform                 |                    |
| Doctrine\DBAL\Schema\Schema                            |                    |
| Doctrine\Migrations\AbstractMigration                  |                    |
| Doctrine\Persistence\ObjectManager                     |                    |
| FILTER_VALIDATE_INT                                    | ext-filter         |
| filter_var                                             | ext-filter         |
| GuzzleHttp\Psr7\Uri                                    |                    |
| GuzzleHttp\Psr7\UriResolver                            |                    |
| Hateoas\Configuration\Route                            |                    |
| Hateoas\Representation\Factory\PagerfantaFactory       |                    |
| Http\Client\Common\HttpMethodsClient                   |                    |
| Http\Client\Common\PluginClient                        |                    |
| Http\Client\Common\Plugin\ErrorPlugin                  |                    |
| Http\Client\Exception\RequestException                 |                    |
| Http\Client\HttpClient                                 |                    |
| Http\Discovery\MessageFactoryDiscovery                 |                    |
| Http\Message\MessageFactory                            |                    |
| Imagick                                                | ext-imagick        |
| JMS\Serializer\SerializationContext                    |                    |
| JMS\Serializer\SerializerBuilder                       |                    |
| MOBIFile                                               |                    |
| PhpAmqpLib\Message\AMQPMessage                         |                    |
| Psr\Http\Message\ResponseInterface                     |                    |
| Psr\Log\LoggerAwareInterface                           |                    |
| Psr\Log\LoggerInterface                                |                    |
| Psr\Log\NullLogger                                     |                    |
| RulerZ\RulerZ                                          |                    |
| Swift_Mailer                                           |                    |
| Swift_Message                                          |                    |
| Symfony\Bundle\MakerBundle\MakerBundle                 |                    |
| Twig\Environment                                       |                    |
| Twig\Extension\AbstractExtension                       |                    |
| Twig\Extension\GlobalsInterface                        |                    |
| Twig\TwigFilter                                        |                    |
| Twig\TwigFunction                                      |                    |
+--------------------------------------------------------+--------------------+

Here the result after this PR:

The following 9 unknown symbols were found:
+--------------------------------------------------------+--------------------+
| Unknown Symbol                                         | Guessed Dependency |
+--------------------------------------------------------+--------------------+
| DAMA\DoctrineTestBundle\DAMADoctrineTestBundle         |                    |
| Doctrine\Bundle\FixturesBundle\DoctrineFixturesBundle  |                    |
| Doctrine\Bundle\FixturesBundle\Fixture                 |                    |
| Doctrine\Common\DataFixtures\DependentFixtureInterface |                    |
| Imagick                                                | ext-imagick        |
| MOBIFile                                               |                    |
| Swift_Mailer                                           |                    |
| Swift_Message                                          |                    |
| Symfony\Bundle\MakerBundle\MakerBundle                 |                    |
+--------------------------------------------------------+--------------------+

Remaining unknown symbols are the ones:

  • that are used by tests and dev only
  • that are optional (Imagisk)
  • that not namespaced (MOBIFile and Swiftmailer) but those are required explicitly already (maybe a bug in the tool with not namespaced classes)

@yguedidi yguedidi marked this pull request as draft May 21, 2022 21:51
@j0k3r j0k3r changed the base branch from master to 2.6.0 May 23, 2022 05:58
@j0k3r j0k3r added this to the 2.6.0 milestone May 23, 2022
@yguedidi yguedidi force-pushed the remove-transitive-dependencies branch from 7a97124 to 4b02400 Compare June 5, 2022 20:59
@yguedidi yguedidi marked this pull request as ready for review June 5, 2022 21:04
@yguedidi
Copy link
Contributor Author

yguedidi commented Jun 5, 2022

@Kdecherf @nicosomb @j0k3r this is ready to be merged! I think it should be merged first, then I'll rebase #5782, then #5759, and we'll be done with my PRs updating composer.lock haha

@yguedidi
Copy link
Contributor Author

yguedidi commented Jun 5, 2022

and here the dependencies update detail:

[old table]

EDIT: no more dependency upgrade as it seems to break a test

@yguedidi yguedidi mentioned this pull request Jun 5, 2022
3 tasks
@j0k3r
Copy link
Member

j0k3r commented Jun 9, 2022

I just merged master into the 2.6.0 branch

@yguedidi yguedidi force-pushed the remove-transitive-dependencies branch from 4b02400 to 216b5f1 Compare July 31, 2022 22:44
@yguedidi
Copy link
Contributor Author

@j0k3r @nicosomb @Kdecherf I'm back! sorry for my absence! I managed to update this, next one is #5782 after this is merged

@j0k3r
Copy link
Member

j0k3r commented Aug 8, 2022

Do we really need 22 commits for changing 2 files?

@yguedidi
Copy link
Contributor Author

yguedidi commented Aug 8, 2022

@j0k3r "need" I would say no, I made it this way to isolate each update.
I can squash to one commit if you prefer

@Kdecherf
Copy link
Member

Kdecherf commented Aug 8, 2022

@yguedidi you can squash all commits changing composer files, I'll merge after that

@yguedidi yguedidi force-pushed the remove-transitive-dependencies branch from 216b5f1 to e881b9d Compare August 8, 2022 13:37
@yguedidi
Copy link
Contributor Author

yguedidi commented Aug 8, 2022

@Kdecherf done

@Kdecherf Kdecherf merged commit bac713c into wallabag:2.6.0 Aug 8, 2022
@Kdecherf
Copy link
Member

Kdecherf commented Aug 8, 2022

Thanks @yguedidi 👍

@yguedidi yguedidi deleted the remove-transitive-dependencies branch August 8, 2022 13:45
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.

None yet

3 participants