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

Dont overwrite .htaccess and web.config on Upgrade #15576

Closed
wants to merge 3 commits into from
Closed

Dont overwrite .htaccess and web.config on Upgrade #15576

wants to merge 3 commits into from

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Apr 26, 2017

Closes #15542

Steps to reproduce the issue

Expected result

Dont do this:

if (file_exists($root . '/htaccess.bak'))

Actual result

htaccess.bak renamed to .htaccess on update overwriting valid .htaccess ?

System information (as much as possible)

Additional comments

https://twitter.com/joovlaki/status/856956148133580801
@brianteeman @nikosdion

Pull Request for Issue # .

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Closes  #15542

### Steps to reproduce the issue




### Expected result

Dont do this: https://github.com/joomla/joomla-cms/blob/4e13f90b67ab461a7e399dc7183441c68869bbc4/administrator/components/com_joomlaupdate/restore.php#L7882

### Actual result

htaccess.bak renamed to .htaccess on update overwriting valid .htaccess ?

### System information (as much as possible)



### Additional comments
https://twitter.com/joovlaki/status/856956148133580801
@brianteeman @nikosdion
@wilsonge
Copy link
Contributor

I would imagine that this https://github.com/PhilETaylor/joomla-cms/blob/ff54615dbb726c53c3952e09435ab75c590825cc/administrator/components/com_joomlaupdate/restore.php#L6429-L6432 and the associated javascript change mentioned in the comment above would also need to be changed?

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

I have no clue :D I just read the comment above that array

@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Contributor

zero-24 commented Apr 26, 2017

Since when do edit 3rd party code?

@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Contributor

zero-24 commented Apr 26, 2017

restore.php is part of Joomla Core, it is contributed code, but very much core code!

restore.php is 3rdparty code.

https://github.com/PhilETaylor/joomla-cms/blob/staging/administrator/components/com_joomlaupdate/restore.php#L1-L11

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@mbabker
Copy link
Contributor

mbabker commented Apr 26, 2017

EIther we as a project are forking and taking on the maintenance of this code on our own or we will continue to integrate it as a third party resource the same as all third party resources are. Just because it is used within and distributed with Joomla does not mean Joomla "owns" it. Unless you're now trying to claim that applies to all third party libraries, I hear Symfony protects their trademark pretty well if you really want to go that far.

@PhilETaylor

This comment was marked as abuse.

@mbabker
Copy link
Contributor

mbabker commented Apr 26, 2017

This particular behavior, while annoying as all can be, isn't a bug in his code. It's really more of an undocumented and undesired behavior for our use case. Yes it probably is something that deserves our attention, but we need to be careful about how many changes we are making to non-Joomla core code (as in anything that is originally sourced from a third-party vendor, including this file).

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

@mbabker

So is this going to be tested and merged? or shall I just close and delete it ?

@nikosdion
Copy link
Contributor

Um. Ahem. Hi. I'm still here. Last time I checked I own the copyright to my code and I didn't sign any paperwork transferring it to the Joomla! Project. Also last time I checked restore.php (properly called Akeeba Restore as you can see in the header) was an integral part of Akeeba Kickstart which is used by millions of people every year to transfer their sites and has a public repository so that other Open Source developers could discuss issues with me and / or submit a pull request. Also last time I checked I was adding features to that code for Joomla!, features which are irrelevant (and useless) to my own software which BTW are 20 LOCs down the line from where you butchered the guts out of that file. Adding ad-hoc changes to that file will only cause pain when you need to update that file.

Regarding the first hunk in this PR, I think you didn't spend much time reading the code or understanding the update process. The initial rename of the .htaccess and other files is a configurable option under the key kickstart.setup.renamefiles. This is something set up by JoomlaupdateModelDefault::createRestorationFile. So instead of butchering the file you could very elegantly edit the model to pass an empty array for that parameter. Simple:)

As for the second hunk in this PR, this is something that if you asked nicely I could very easily integrate in Akeeba Restore as a parameter which would be passed by the Model to disable those automatic renames. It's not a bug in want of a fix, it's a necessary feature for restoring sites which has a side effect when updating sites. I don't understand the derision about fixes, though, and I find it unfair and hurtful. The legitimate bugs you have reported, Phil, were fixed in a matter of hours. Moreover I get to reply to your clients when you CC me on your communication with them, despite the vast majority not being bugs and half of these people not being my paid clients. But I digress. I'm here to help millions of Joomla! users get updates, not psychoanalyze people.

Here's my problem. So far you guys have done ad-hoc changes to that file without caring much to PR upstream. If we tried to update Akeeba Restore I'm sure stuff would break. If someone can PR the relevant changes (@mbabker pretty please?) then I can of course add the missing feature in Akeeba Engine and make a PR back to Joomla! to integrate it to Joomla! Update, fixing the very real issue you are trying to address here. In short, you do the Right Thing(tm), I do the Right Thing(tm), i.e. we do it the Open Source way. IMHO that works much better than accusing each other ;)

@nikosdion
Copy link
Contributor

Nvm. I backported everything and implemented the feature. Apparently all the work done in 2015 and 2016 to clean up typos was overwritten in October 2016 when we updated restore.php. These are the perils of not making PRs upstream. No worries, I backported these cosmetic fixes too.

Now, do you want me to fix this issue? It's as simple as adding two lines in the model and replacing restore.php with a new version.

Oh, BTW, that "Kickstart crap" you mention is how Joomla! Update talks to Akeeba Restore without going through Joomla! because, you know, while the update is running you cannot run Joomla! (the Joomla! application is in a half-updated state). I am disinclined to rename the configuration parameters for the simple reason that they are used in a lot of my software. This is the kind of change that has a tangible financial impact by means of sunk time and the potential to introduce subtle bugs but also ABSOLUTELY ZERO functional impact in this specific use case (Joomla! Update). Therefore I won't implement it. It's my time and I choose to spend it where it matters, i.e. where it will improve people's lives.

@mbabker
Copy link
Contributor

mbabker commented Jul 28, 2017

If you would add support for parameters that would be beneficial.

@PhilETaylor PhilETaylor deleted the patch-2 branch July 29, 2017 07:51
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

8 participants