lotgd / core

Core functionality for Legend of the Green Dragon, a text-based RPG game.
GNU Affero General Public License v3.0
152 stars 15 forks source link

Refactor BootConfiguration into LibraryConfiguration #55

Closed austenmc closed 8 years ago

austenmc commented 8 years ago

Read all configuration for modules from LibraryConfiguration.

Removed rootDir support b/c no one seems to be using it. Not sure what this was for and it was making the code quite complex.

Also removed the tests for the verification feature of ModuleManager b/c the code needs more refactoring before its easy to write tests. Im planning to break this verification into a separate set of classes anyway.

Vassyli commented 8 years ago

I put in rootDir support exactly because i need it for the crate:

The cwd in symfony is not the same as the crate root directory (it's crate-graphql/web, not crate-graphql/). That's why I need to start the service with a different root directory.

Vassyli commented 8 years ago

I'm okay with the name change, but not with rootDir removal.

austenmc commented 8 years ago

Let's talk about why you were using the $rootDir support:

  1. Finding the config file by concatenating the $rootDir w/ LOTGD_CONFIG path. This would be a non-standard way of specifying a path in an environment variable. The env variable path should be either relative to the crate's cwd (which the crate author knows about and doesn't need to specify) or an absolute path. That's the way this PR makes it (check out the changes to Configuration).
  2. Finding the autoloader. The current code will work for your crate, since ../vendor will be the autoloader, but this is just a coincidence. Let me change the code to use $composer->getConfig()->get('vendor-dir') which should be the right location of the autoload.php file.

Given these two comments, do you still think you need $rootDir support?

Vassyli commented 8 years ago
  1. I thought we decided to move away from environment variables if possible. The core is just a package used by a crate and therefore should be configurable by the crate itself, not by environment variables set by the user (the crate knows where the config files are which can be set by the author, but setting up environment variables is server-dependent).
  2. It's an oversight on my side that ComposerManager->getComposer() does not respect $rootDir, but searches on it's own. We should change that, not remove $rootDir. Without this, composer automatically looks for a composer.json file in the cwd and cannot find it if called from a different cwd than $rootDir.
  3. Explicitly giving a $rootDir is superior to extracting the path from an unrelated variable.
  4. Calling vendor/bin/daenerys needs a different environment variable than accessing web/app.php since the cwd is not the same. Normalizing it via web/app.php (indirectly via the service created by symfony in this case) is a necessity.

So yeah, I still think we need $rootDir.

austenmc commented 8 years ago

(Forgot the library configuration one, will fix tomorrow)

austenmc commented 8 years ago

@Vassyli: Ok I think this is ready for final review. crate-graphl tests pass and it loads the index page in symfony. It does not load /graphiql, is that expected?

Vassyli commented 8 years ago

Which branch did you test it out? Master isn't compatible anymore. Is it a routing error?

Vassyli commented 8 years ago

Update: Oh, yeah, that's to be expected, I've forgotten to change www to grpahql in the GraphiQL Controller.

Vassyli commented 8 years ago

Can you test if vendor/bin/daenerys works as well as expected?

austenmc commented 8 years ago

I checked daenerys, it worked. And I was working on the bin-ext branch. Pushing this.