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

Error when using without Altis #497

Closed
mattheu opened this issue Jun 23, 2022 · 11 comments · Fixed by #575
Closed

Error when using without Altis #497

mattheu opened this issue Jun 23, 2022 · 11 comments · Fixed by #575
Labels
bug Existing functionality isn't behaving as expected

Comments

@mattheu
Copy link
Member

mattheu commented Jun 23, 2022

We're using Altis local server as a standalone local development environment for a non-altis project. This is working great with v11.0.1 but is broken in v11.0.4.

Whilst I understand this isn't officially 'supported' it is documented in the readme of this project as a way it can be used, so I'd not expect to have a hard dependency on Altis core.

Steps to reproduce:

Install local server as a standalone environment. As documented in the readme. Run composer server start.

I then get the following error:

Call to undefined function Altis\get_config()

Note this worked fine on v11 but there has been a breaking change in a security/patch update.

@mattheu mattheu added the bug Existing functionality isn't behaving as expected label Jun 23, 2022
@Sephsekla
Copy link

Just ran into this issue myself while trying out using local server to test WordPress 6.0.

The problematic line is https://github.com/humanmade/altis-local-server/blob/master/inc/composer/class-docker-compose-generator.php#L843, I was able to workaround locally by changing the following:

		$modules = Altis\get_config()['modules'] ?? [];

⬇️

		$modules = [];

		if ( function_exists( 'Altis\\get_config' ) ) {
			$modules = Altis\get_config()['modules'] ?? [];
		}

Happy to open a PR if the Altis team are OK with this as a fix.

@mattheu
Copy link
Member Author

mattheu commented Jul 8, 2022

Whilst this stops the blocking error, the problem with this suggestion @Sephsekla is that it doesn't give you the exact same functionality that you had before this breaking change was introduced.

Looking at 21639f5, it used to read from composer.json. So probably we need something like

		if ( function_exists( 'Altis\\get_config' ) ) {
			$modules = Altis\get_config()['modules'] ?? [];
		} else {
			$json = file_get_contents( $this->root_dir . DIRECTORY_SEPARATOR . 'composer.json' );
			$composer_json = json_decode( $json, true );
			$modules = ( $composer_json['extra']['altis']['modules'] ?? [] );
		}

@Sephsekla
Copy link

Oh that's a good point - I was just using it with WP Core so hadn't considered that.

What's the use case for loading Altis modules but not Altis itself? At that point are you not essentially using Altis with some custom configuration applied?

@robindevitt
Copy link
Contributor

I came across the same issue and did the following steps to get me going:

  • composer update -W
  • composer self-update and then composer update
  • composer server start

@kovshenin
Copy link
Contributor

Looks like it was broken in #484 which made it to 11.0.3 and 10.0.9.

@roborourke
Copy link
Contributor

@mattheu @Sephsekla you can require Altis Core as a dev requirement only I think. If there are reasons not to do this and you can come up with a reasonable alternative then it might be something that can be included

@Sephsekla
Copy link

@roborourke even if that is the case, it feels a bit unnecessary to require Altis Core in all cases - fair enough if Altis modules are being used, but if not then I think a fallback is reasonable.

If you do feel strongly that it should require Altis Core locally, then should that requirement not be set in the composer.json?

@rmccue
Copy link
Member

rmccue commented Jul 14, 2022

fair enough if Altis modules are being used, but if not then I think a fallback is reasonable

Altis Local Server is designed for usage in Altis, and nothing else. While it's usable for other purposes and we're not intending to intentionally break anything unnecessarily, any usage outside of Altis isn't something we're going to factor into how it's built and designed.

Loading data directly from composer.json via file_get_contents() isn't something we're going to add; we already have a function to do that in altis/core which we're using.

Provided it doesn't complicate our release process, I think we can add altis/core as a dependency though.

@roborourke
Copy link
Contributor

If you do feel strongly that it should require Altis Core locally, then should that requirement not be set in the composer.json?

Yeah if there was anything breaking about this change it was that I didn't add the core module as a dep of this package, or at least as a peer dependency suggestion, I hadn't considered usage outside of Altis at the time and it made sense to consolidate on how configs are read to enable CI env config overrides.

@Sephsekla
Copy link

Altis Local Server is designed for usage in Altis, and nothing else. While it's usable for other purposes and we're not intending to intentionally break anything unnecessarily, any usage outside of Altis isn't something we're going to factor into how it's built and designed.

That's absolutely fair, and I'm aware that using for core WP isn't the main use case. That being said, if the readme offers this as an option then surely it should be considered to some degree.

I understand that the Altis team can't provide support for non-Altis usage, but are you open to contribution/PRs etc for this kind of thing if it won't negatively impact the core Altis usage?

@roborourke
Copy link
Contributor

I understand that the Altis team can't provide support for non-Altis usage, but are you open to contribution/PRs etc for this kind of thing if it won't negatively impact the core Altis usage?

Yep, sorry I wasn't super clear about that.

That's absolutely fair, and I'm aware that using for core WP isn't the main use case. That being said, if the readme offers this as an option then surely it should be considered to some degree.

There's a lot to keep track of, and I wasn't aware of that documentation. We probably should have pushed back in #152 or requested some changes. It was never a capability actively designed for or intended to be maintained, more of a happy accident that it did work standalone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing functionality isn't behaving as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants