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 the md5_file() check, which hashes contents. #2065

Closed

Conversation

amcgowanca
Copy link

@amcgowanca amcgowanca commented Oct 1, 2017

Problem

In conjunction with issue #2064 , I uncovered that it is not possible to override the composer.*.json files which are copied into [repo.root]/blt/ at time of install or update. This is primarily due to the md5_file() being invoked which hashes the contents of the files themselves. Therefore, if files are not identical, the BLT provided composer.*.json are copied into [repo.root]/blt/ which overwrites existing ones.

Note: I may have overlooked something, but based on the PHP source file of md5.c, both md5_file and md5 perform the same operations except where md5_file opens the specified file to stream and hash the contents.

Proposed changes

  • Remove the need to check content hash and simply perform a "if file does not exist" condition, then copy.

Usefulness

@grasmash
Copy link
Contributor

grasmash commented Oct 1, 2017

These files are not intended to be modified. You should not customize them at all. This is a necessary part of BLT is composer update mechanism.

If you would like to modify your application’s composer dependencies then you may do it directly in the root composer file. If you do not want to include these suggested and required packages, you can remove the merge configuration that specifies them from your composer root file.

@amcgowanca
Copy link
Author

@grasmash , Thanks!

amcgowanca added a commit to amcgowanca/rhino-project that referenced this pull request Oct 1, 2017
@amcgowanca
Copy link
Author

@grasmash ,

Thinking about this a bit more. I suspect that there is never any intention to use the composer.suggest.json to contain upgrade/update dependencies except when the files match and therefore the composer.required.json is what is primarily used for containing the actual BLT dependencies during updates. No?

Would it not make sense to ensure that the composer.required.json is always updated (therefore copied when the hashes do not match) but not necessarily the composer.suggest.json. Am I missing something?

@greylabel
Copy link
Contributor

greylabel commented Oct 1, 2017

One other related issues I am running into is that BLT includes a failing patch in composer.required.json. This potentially could be problematic when using BLT for core and/or contrib development. It might be worth considering placing all patches to Drupal core and contrib as suggested so they can be easily removed when still wanting to include the packages defined in composer.required.json.

@grasmash
Copy link
Contributor

grasmash commented Oct 2, 2017

It is intended that both of these files always match and that they be fully managed by BLT. This is why the files are git ignored by default.

Let me give a brief background of the purpose of these files. Composer itself does not provide a mechanism for BLT to provide blt-based applications with require-dev requirements. It also does not provide a mechanism for us to provide default yet optional packages. For this reason, we use the Wikimedia merge plug-in to merge in the require and require-dev arrays that BLT projects should have.

We have split these packages into two categories: required (those without which BLT would fail to operate) and suggested.

Depending on the version of BLT that you are using, we need to define different versions of the required and suggested packages. Regardless of whether they are required or suggested, we need to have control over the version constraints for those packages. When BLT is updated, those constraints need to be updated. We do not want to change values in custom files to achieve this.
So, these files need to be maintained by BLT itself and should not be interfered with by the client application.

Initially, we placed these files in the vendor directory so that they could not be tampered with. However, this led to strange edge cases with Composer (having to do with the order of operations for configuration merge and package updates), so these files were moved to the blt directory and gitignored. But unfortunately does make them tempting for clients to modify.

If any client projects need to override the packages or version constraints for the required or suggested packages, they can either stop merging in these files, or they can simply define an override in their root composer.json file. There shouldn't be any reason to modify composer.required.json or composer.suggest.json since their values can easily be overridden in this way.

I would be open to modifying the architecture or the wording such that this is more clear. For instance, we can place these in a "do-not-modify" directory or detect invalid customizations to these files and throw an informative exception.

@grasmash
Copy link
Contributor

grasmash commented Oct 2, 2017

I'm going to close this pull request, as I do not believe we should change this behavior. However, feel free to open an issue to discuss modification to file naming or other conventions if you feel that would be helpful.

@grasmash grasmash closed this Oct 2, 2017
@amcgowanca
Copy link
Author

Thanks @grasmash for the explanation! Much appreciated! It was really just so that for spinning up quick instances without Lightning could be achieved without having to specifying or performing a handful more operations such as physically removing the composer.suggested.json and such :)

@thom8
Copy link
Contributor

thom8 commented Oct 4, 2017

It might actually be useful to set an extra config value in your root composer.json to disable this behaviour, with the disclaimer than you may require manual updates when upgrading BLT, hence this should be disabled by default

@thom8
Copy link
Contributor

thom8 commented Oct 4, 2017

To be completely honest I've had a nightmare of a time managing dependencies within a BLT project due to these composer includes and understanding the risks would happily self manage this these if possible.

Compared to another site built using https://github.com/drupal-composer/drupal-project which is super easy to manage.

It also conflicts with https://www.dependencies.io/ which I've started using to automate updates.

@thom8
Copy link
Contributor

thom8 commented Oct 4, 2017

@amcgowanca you could potentially manage changes to these files via patches, as the md5 hash would match the patched version, but I'd still prefer a way to bypass this completely.

@amcgowanca
Copy link
Author

@thom8 Really it was just for myself for spinning up BLT instances without Lightning and another set of contribs a bit faster, to be honest. Instead of using a composer create-project ... command followed by a handful of additional steps for removing Lightning itself was essentially what I was originally looking at achieving. :)

@thom8
Copy link
Contributor

thom8 commented Oct 4, 2017

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.

None yet

4 participants