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

Removed orphan directory and code of compiler and downloder. #1667

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented May 30, 2021

Description (*)

Related Pull Requests

PR #534 and PR #952.

Fixed Issues (if relevant)

  1. Fixes issue Remove orphan directory and code #1666.

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

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)

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label May 30, 2021
@kiatng kiatng added the Cleanup label May 30, 2021
fballiano
fballiano previously approved these changes May 31, 2021
@fballiano
Copy link
Contributor

I love these kinds of PRs!

@addison74
Copy link
Contributor

addison74 commented Jun 3, 2021

I strongly recommend not removing /includes directory. Inside of it there is an important file named config.php that can help you to achieve some unknown features. For example, if you are using this combination HAProxy (for SSL Offloading) + Varnish + Apache in Magento Backend -> Orders page you will get a chain of IP's addresses, more precisely 3 IP's because the offloading part of Magento was never pushed to a final shape. Recently Colin Mollenhour managed to offer a correct solution for this. In order to get only one IP address listed in Orders page I am using config.php. Here is an example from production:

if(isset($_SERVER['HTTP_X_REAL_IP'])) {
    $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_REAL_IP'];
    $_SERVER['HTTP_X_FORWARDED_FOR'] = '';
}

HTTP_X_REAL_IP comes from .htaccess where I am processing the IP's to get the remote one. In conclusion, config.php allows you to set many things to change what Magento gets.

@fballiano
Copy link
Contributor

I strongly recommend not removing /includes directory. Inside of it there is an important file named config.php that can help you to achieve some unknown features. For example, if you are using this combination HAProxy (for SSL Offloading) + Varnish + Apache in Magento Backend -> Orders page you will get a chain of IP's addresses, more precisely 3 IP's because the offloading part of Magento was never pushed to a final shape. Recently Colin Mollenhour managed to offer a correct solution for this. In order to get only one IP address listed in Orders page I am using config.php. Here is an example from production:

if(isset($_SERVER['HTTP_X_REAL_IP'])) {
    $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_REAL_IP'];
    $_SERVER['HTTP_X_FORWARDED_FOR'] = '';
}

it's interesting but in my opinion it seems the wrong place to put it (although I totally see your point). If IP chains are necessary to handle, then let's put it in the index.php. includes/config.php is 99% unused (while causing I/O) and had a totally different purpose.

@addison74
Copy link
Contributor

I have nothing against as long as what I said above can be obtained in another way. Magento needs to be able to deal with such server configurations that when it was born in 2008 were not possible or accessible to anyone. I will even propose in the Discussions section on this topic.

@colinmollenhour
Copy link
Member

I think the include for includes/config.php was already removed from index.php but it was missed from index.php.sample somehow.. Should this sample file not just be deleted as well? So already to use config.php you would have to have a modified index.php. But I agree this file is not the right place to add custom code so as a project we should consider it dead code that is safe to remove.

One minor problem with this PR is if existing installations have important files in includes directory (no idea why this would be the case) these will be potentially exposed by Apache because the .htaccess file is removed.. So maybe the .htaccess file should remain in the 1.9.4.x branch and only be removed in 20.0?

@kiatng kiatng changed the base branch from 1.9.4.x to 20.0 June 4, 2021 01:37
@kiatng kiatng dismissed fballiano’s stale review June 4, 2021 01:37

The base branch was changed.

@kiatng kiatng changed the base branch from 20.0 to 1.9.4.x June 4, 2021 01:38
@kiatng
Copy link
Contributor Author

kiatng commented Jun 4, 2021

As pointed out by @addison74, this PR may break BC. I tried to edit the base branch to 20.0 but that didn't work as it brought along some 35 other commits. Not sure how to do this, do I close this PR and make a new one for branch 20?

@Flyingmana
Copy link
Contributor

you can wait till next release, then all of these commits are already part of 20.0, so changing the branch will not bring them up anymore

@kiatng kiatng changed the base branch from 1.9.4.x to 20.0 June 27, 2021 02:25
@Flyingmana Flyingmana merged commit 10da7af into OpenMage:20.0 Jul 30, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  ±0  6 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 10da7af. ± Comparison against base commit 3fbc99a.

sreichel added a commit that referenced this pull request Dec 15, 2022
@kiatng kiatng deleted the remove_orphan_compiler_downloader branch February 27, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants