ffraenz / private-composer-installer

Composer install helper outsourcing sensitive keys from the package URL into environment variables
MIT License
227 stars 16 forks source link

update ACF example #42

Closed macbookandrew closed 3 years ago

macbookandrew commented 3 years ago

As currently documented with version 1.2.3, the command composer require "advanced-custom-fields/advanced-custom-fields-pro:*" fails.

This brings the version up to something close to current so it doesn’t immediately fail and discourage use of this package.

An alternative might be to use a placeholder instead of 1.2.3 so it’s a bit more obvious that the user needs to set it to a current version.

szepeviktor commented 3 years ago

Oh nooooooooooooooooooooooooooo, @macbookandrew!

kép

That code snippet is in the Examples section of the README.

szepeviktor commented 3 years ago

I know - I know... Copypaste guys are in trouble 😢

macbookandrew commented 3 years ago

What do you think of this to make it more immediately obvious?

-    "version": "1.2.3",
+    "version": "{enter a specific version number}",
szepeviktor commented 3 years ago

I think copypasting before acquiring knowledge is something I no not dare to comment!

szepeviktor commented 3 years ago

On one hand supporting copypasters makes web development an even worse profession. On the other hand giving a valid version makes the package more popular.

💣

macbookandrew commented 3 years ago

I sympathize with the dilemma. Maybe something like this would be better to encourage people to read more closely?

-    "version": "1.2.3",
+    "version": "1.2.3", # Note: you must replace this version number

The example looks like a valid copy/pasteable snippet and when it fails throwing a Composer\Download\TransportException, it’s not apparent that it’s the user’s fault for blindly copy/pasting, possibly leading users to falsely assume this package is at fault.

Whatever you think; it’s your package 🙂

Thanks for your work on it!

szepeviktor commented 3 years ago

a valid copy/pasteable snippet

Actually the way you upgrade ACF is to edit composer.json, so when installing ACF the first time you should set the most recent version from ACF's changelog.

it’s your package 🙂

It's @ffraenz's.

macbookandrew commented 3 years ago

Well then…it’s up to @ffraenz and I thank him for his work on it 🙂

I’m just hoping to help those not as experienced as you or I so they can learn something rather than assuming this is broken. Have a nice day 👋🏻

szepeviktor commented 3 years ago

to help those not as experienced

Please consider adding a new section instead.

Upgrade

This is how you upgrade a package installed by private-composer-installer ...

  1. manual/scripted watch of new releases
  2. update version constrain as needed
  3. update version in package definition
macbookandrew commented 3 years ago

Yeah, that would be helpful too

ffraenz commented 3 years ago

Hi @macbookandrew, thank you for opening an issue on this. I see why the examples section might cause confusion or even frustration. I don't want to mention a specific ACF version in the README as we would need to keep it updated afterwards. Introducing a new placeholder format like {placeholder} could be confusing, too, as people could mistake it with the placeholders used in the dist URL. As comments are not allowed in JSON I would suggest to replace 1.2.3 by something like REPLACE_BY_LATEST_ACF_VERSION. For consistency let us also change the version in the 'Arbitrary private package' section.

@szepeviktor Maybe we could add an FAQ section to the README to include questions like 'How to update a package?'. We could also discuss this in a separate issue or pull request.

macbookandrew commented 3 years ago

I don't want to mention a specific ACF version in the README as we would need to keep it updated afterwards.

That makes total sense.

I updated the PR with your requested changes. Thanks!

ffraenz commented 3 years ago

@macbookandrew Thanks again!

I will have a look at the failing tests in a later session. As we only updated README, it is not related to this PR.

macbookandrew commented 3 years ago

@ffraenz any chance you could add hacktoberfest-accepted label to this?

ffraenz commented 3 years ago

Yeah, sure :)