Skip to content

Conversation

@lats1
Copy link
Contributor

@lats1 lats1 commented Aug 3, 2021

Upgrading the vlucas/dotenv library to match the requirement contraint in the forloeb module for the same library.

@lats1 lats1 requested review from andriyun and madsnorgaard August 3, 2021 12:42
Copy link
Contributor

@madsnorgaard madsnorgaard left a comment

Choose a reason for hiding this comment

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

I notice that this is being merged directly to Master branch. Should it have been develop instead?

@lats1 lats1 changed the base branch from master to develop August 13, 2021 08:23
@lats1
Copy link
Contributor Author

lats1 commented Aug 13, 2021

@madsnorgaard Yes this should go into the develop branch. My mistake and I have changed the base.

@lats1 lats1 requested a review from madsnorgaard August 13, 2021 08:24
Copy link
Contributor

@madsnorgaard madsnorgaard left a comment

Choose a reason for hiding this comment

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

I approve the changes, lets merge them.

Copy link
Contributor

@andriyun andriyun left a comment

Choose a reason for hiding this comment

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

Hi @lats1
Very good PR. I noticed few small things that we can get improved in my opinion.

catch (InvalidPathException $e) {
// Do nothing. Production environments rarely use .env files.
}
$dotenv = Dotenv::createImmutable(__DIR__);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this file to the one we have in Drupal composer project?
https://github.com/drupal-composer/drupal-project/blob/9.x/load.environment.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. As the os2forms_forloeb module has a dependency to the dotenv library as well, this pr OS2Forms/os2forms_forloeb#67 needs to be addressed first and a part of the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now dependant on the following PR OS2Forms/os2forms_forloeb#66

Copy link
Contributor

Choose a reason for hiding this comment

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

@lats1
PR
OS2Forms/os2forms_forloeb#66 has been merged :)

@andriyun
Copy link
Contributor

@lats1 I've resolved conflict that has appeared after applied security updates

@andriyun
Copy link
Contributor

@lats1 Does my review comments make sense for you?
If yes then I can update it and we can get it merged.

Otherwise I'd like to hear your point of view why we can't do it :)

@andriyun
Copy link
Contributor

andriyun commented Sep 3, 2021

Hi @lats1, @madsnorgaard
How could we get the changes above handled?

For me, it looks like we stuck with this suggestion.

Wouldn't it be a good idea to merge it as it is, and then get it updated to a recent version later?

@madsnorgaard
Copy link
Contributor

madsnorgaard commented Sep 3, 2021

Did a few releases yesterday and could finally run a composer update. Lets se if that does not solve this stalemate :)

Copy link
Contributor

@andriyun andriyun left a comment

Choose a reason for hiding this comment

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

Looks good.
So we only missing recent changes for load.environment.php file. See comment above

@lats1 lats1 requested a review from andriyun September 8, 2021 10:52
Copy link
Contributor

@andriyun andriyun left a comment

Choose a reason for hiding this comment

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

Awesome @lats1
Thank you

@andriyun andriyun merged commit 6d0ccdd into develop Sep 8, 2021
@andriyun andriyun mentioned this pull request Sep 8, 2021
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.

4 participants