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

Allow install on PHP8 #140

Merged
merged 12 commits into from
Jun 29, 2021
Merged

Allow install on PHP8 #140

merged 12 commits into from
Jun 29, 2021

Conversation

snapshotpl
Copy link
Contributor

No description provided.

@snapshotpl
Copy link
Contributor Author

This PR need support for laminas PHP8 support first.

@roelvanduijnhoven
Copy link
Collaborator

@snapshotpl I noticed a lot of new Laminas release: could you re-test your PR? If so: let me know, and I'll happily merge your PR.

@snapshotpl
Copy link
Contributor Author

Mvc and servicemanager still waiting for php 8 support.

@NielsdeBlaauw
Copy link

Hi @snapshotpl

With the release of servicemanager: 3.6.0 and mvc:3.2.0, PHP8 support is available for all required packages. The current version constraints in this PR should permit all packages to update to a php8 compatible version.

Would you reconsider merging this PR?

Cheers.

@roelvanduijnhoven
Copy link
Collaborator

@NielsdeBlaauw and @snapshotpl It looks like we can release a PHP 8.x compatible version :).

Before we merge: can you update the .travis.yml file so that the TravisCI file does not show failure when the nightly fails? Apart from that it looks good to be merged!

Please ping me whenever this is ready to go, and I'll make sure to merge and release a new version. I guess this can be a minor release as the PHP version was not bumped?

@snapshotpl
Copy link
Contributor Author

Looks like we need support php 8 for https://github.com/aws/aws-sdk-php-zf2 :-(

@roelvanduijnhoven
Copy link
Collaborator

Mm @snapshotpl I am looking how we use that library. Actually we don't depend on it that hard. And it is an optional dependency.

You will find that only the test code uses AwsModule. The only requirement really is if you look at https://github.com/JouwWeb/SlmMail/blob/master/src/SlmMail/Factory/SesServiceFactory.php is that the Aws\Sdk service in the registry should return a Aws\Ses\SesClient. We can also simply list that as an instruction in the docs? That would remove the dependency altogether.

@snapshotpl
Copy link
Contributor Author

In test you can see I downgrade composer to version 1. It's because AwsModule require outdated laminas-dependency-plugin aws/aws-sdk-php-zf2#73

@snapshotpl
Copy link
Contributor Author

Scrutinizer fail on phpcs, but it pass on travis

@roelvanduijnhoven
Copy link
Collaborator

@snapshotpl First of all: thank you for your effort. So the AWS package is optional now, great. But.. still it does not work on PHP8, right? So releasing this library for PHP 8.0 would still be kinda weird. Because we know the optional package will not work under PHP8. So I don't like this solution.

@roelvanduijnhoven
Copy link
Collaborator

I would say we either remove the dependency on https://github.com/aws/aws-sdk-php-zf2. Or we try to upgrade that version to PHP 8.0 by a PR.

@snapshotpl
Copy link
Contributor Author

I think is ok now. This problem will be resolved when AWS will be upgraded. For me, when I doesn't use this dependency is win situation.

@roelvanduijnhoven
Copy link
Collaborator

@snapshotpl Thansk for your work allready :). That the dependency is optional is fine. But.. if the new PHP8 version of this repo does not allow the SES adapter to work, we cannot publish it in my opinion.

Valid options I see:

  1. Remove the SES adapter altogether.
  2. Remove dependency on https://github.com/aws/aws-sdk-php-zf2 altogether, and depend on Aws\Sdk directly.
  3. Wait or update the aws-sdk-php-zf2 package.

And really I would like to go with option 2 here. As we allready do so for 99% percent.

Would you be willing to help here? I'll push you in the right direction.

That I think is necessary is that we only need to update file https://github.com/JouwWeb/SlmMail/blob/master/src/SlmMail/Factory/SesServiceFactory.php. And make sure it will construct a client, and populate it with config parameters. Just like done in https://github.com/aws/aws-sdk-php-zf2/blob/master/src/Factory/AwsFactory.php. And in much the same way as https://github.com/JouwWeb/SlmMail/blob/master/src/SlmMail/Factory/MandrillServiceFactory.php.

Psuedo code:

namespace SlmMail\Factory;

use Aws\Sdk;
use Interop\Container\ContainerInterface;
use Laminas\ServiceManager\Factory\FactoryInterface;
use SlmMail\Service\SesService;

class SesServiceFactory implements FactoryInterface
{
    public function __invoke(ContainerInterface $container, $requestedName, array $options = null)
    {
        $config = $container->get('config');

        if (!isset($config['slm_mail']['ses'])) {
            throw new RuntimeException(
                'Config for SES is not set, did you copy config file into your config/autoload folder ?'
            );
        }

        $sdk = new Sdk($config['slm_mail']['ses']);

        return new SesService($sdk->createSes());
    }
}

And add a mail/config/slm_mail.ses.php.dist file with:

<?php

return [
    'slm_mail' => [
        'ses' => [
            'credentials' => [
                'key'    => 'change_me',
                'secret' => 'change_me'
            ],
            'region'   => 'change_me',
            'version'  => 'latest',
            'DynamoDb' => [
                'region'  => 'another_region',
                'version' => 'latest'
            ]
        ]
    ]
];

And I think that should just work.

@snapshotpl
Copy link
Contributor Author

Travis.org is down, probably need to migrate to Travis.com

@snapshotpl
Copy link
Contributor Author

@roelvanduijnhoven I can't disable scrutinizer, it isnt defined in sources, you need to check it in repo settings.

@roelvanduijnhoven
Copy link
Collaborator

I think I removed it us now @snapshotpl.

@snapshotpl
Copy link
Contributor Author

Great so if you are OK with this PR, then ship it!

@snapshotpl
Copy link
Contributor Author

@roelvanduijnhoven all green!

@roelvanduijnhoven roelvanduijnhoven merged commit a53aafa into Webador:master Jun 29, 2021
@roelvanduijnhoven
Copy link
Collaborator

Nice work @snapshotpl! I released an alpha version: https://github.com/JouwWeb/SlmMail/releases/tag/v4.0-alpha.

Let's both test this version. Please ping me in 2 days, and tell me if it is still working. If so: I'll release 4.0 :)!

@snapshotpl
Copy link
Contributor Author

@roelvanduijnhoven just released on production and works like harm! However mailgun adapter, not Aws.

@roelvanduijnhoven
Copy link
Collaborator

@snapshotpl I just released version 4.0. Thank you for all the effort 💪! I validated both Mandrill and Sparkpost are working fine :).

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

Successfully merging this pull request may close these issues.

3 participants