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

Update README install instructions #2981

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

justinbeaty
Copy link
Contributor

@justinbeaty justinbeaty commented Jan 19, 2023

Description (*)

IMO, the install instructions are both unclear and inaccurate. I expanded on the section to explain exactly how to install via composer. I also added in notes about allowing patches which is critical to avoid security vulns.

View formatted README here: https://github.com/justinbeaty/magento-lts/tree/topic-readme-install#installation

Related Pull Requests

OpenMage/OpenMage.github.io#118

Manual testing scenarios (*)

  • Try to install via composer the old way, it doesn't work
  • Try to install via composer the new way, it should work.

Questions or comments

  1. I changed "openmage/magento-lts":"^19.5.0" to "openmage/magento-lts":"^19.4.0" because 19.5.x doesn't exist. Plus, ^19.4.0 allows 19.5.x when it will exist.
  2. Packagist is outdated. It tries to install 19.4.18.
  3. Is it just me, or does the branch alias not work? composer require "openmage/magento-lts":"dev-main" doesn't work for me. I believe there's special consideration when a branch has a numerical name. (Ref)

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel
Copy link
Contributor

Nice.

I'm currently work on refactoring docs. Every improvement is welcome :)

@justinbeaty
Copy link
Contributor Author

Sometimes I feel like it's a secret handshake to know how to install magento-lts via composer. 😉

README.md Show resolved Hide resolved
@@ -61,21 +62,59 @@ __Please be aware that although OpenMage is compatible that one or more extensio

## Installation

### Manual Install

Download the latest [release archive](https://github.com/OpenMage/magento-lts/releases) and extract it over your existing install. **Important:** you must download the ZIP file from a tagged version on the releases page, otherwise there will be missing dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

did we have this part before like this? because extracting it over an existing install is only updating/adding files. But files which got deleted or moved will stay in their place, which in some cases might be a security issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Flyingmana I think it's related to the "composer thing". It is true that just overwriting everything can leave orphaned files, maybe we could write it in a better way. It's still a difficult task since, if people installed everything in the same docroot, how will they be able to "remove everything" and reinstall? They'll for sure have css/js and other stuff in various directories that make everything more difficult.

BTW we have a specific section "Since OpenMage 19.5.0 / 20.1.0" in the readme with a "rm" command, check it out ;-)

Copy link
Member

Choose a reason for hiding this comment

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

what we could do is updating the migration project https://github.com/OpenMage/migrate/blob/master/create-patches.sh to create diff based patches as in the old days. Also github offers patch files for release upgrades as for example https://github.com/OpenMage/magento-lts/compare/v20.0.17...v20.0.18.patch although thats not covering the composer dependencies, so the migration project which creates and hosts patches might be a valuable approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that this is the "classic" way to install / upgrade Magento. Not the best, but is for people who just use FTP.

I wouldn't personally work on creating diff based patches since simply using composer would be a better approach and we should be encouraging that way intead.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

LGTM

@fballiano fballiano merged commit df0ebab into OpenMage:1.9.4.x Jan 24, 2023
@fballiano
Copy link
Contributor

merged and v20ed

@fballiano
Copy link
Contributor

I'm following the instructions but it seems the ZF1 patches are not applied :-(

@justinbeaty
Copy link
Contributor Author

What version of magento-lts are you requiring?

I did:

$ composer require "openmage/magento-lts":"20.0.x-dev"

[...]

  - Installing openmage/magento-lts (20.0.x-dev 2fd3199): Extracting archive
  - Applying patches for shardj/zf1-future
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/MAG-1.1.1.patch (MAG-1.1.1)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/MAG-1.9.3.0.patch (MAG-1.9.3.0)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/MAG-1.9.3.7.patch (MAG-1.9.3.7 - SUPEE-10415)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/MAG-1.9.3.9.patch (MAG-1.9.3.9)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/OM-918.patch (OM-918 - Add runtime cache to Zend_Locale_Data)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/OM-1081.patch (OM-1081 - Not detecting HTTPS behind a proxy)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/OM-2047.patch (OM-2047 - Pass delimiter char to preg_quote)
    https://raw.githubusercontent.com/OpenMage/composer-patches/74862e7a9fa29dd62c5c02d585d931ce9e3c5815/patches/OM-2050.patch (OM-2050 - Prevent checking known date codes)

@justinbeaty
Copy link
Contributor Author

But if I do:

composer require "openmage/magento-lts":"^19.4.0"
# or
composer require "openmage/magento-lts":"^20.0.0"

Then no patches, since it's not in those releases.

@fballiano
Copy link
Contributor

fballiano commented Jan 24, 2023

you're totally right and I'm sorry, what stupid comment I posted 🤦🏻‍♂️
but my brain is boiled cause I'm working on a cool project I'll tell you about probably tomorrow heheheh

@justinbeaty
Copy link
Contributor Author

No problem, I have those moments too. 😅

Also nice! Looking forward to it.

@fballiano
Copy link
Contributor

@justinbeaty #2995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants