Why is this RTC if there are no tests?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4259.

We've generally moved really simple stuff to RTC with just one test or no tests (but with a code review), depending on the code being changed. If you feel that tests are needed for this one, I can get some. No problem either way.

We have?

On 8 October 2014 18:46, Nick Savov notifications@github.com wrote:

We've generally moved really simple stuff to RTC with just one test or no
tests (but with a code review), depending on the code being changed. If you
feel that tests are needed for this one, I can get some. No problem either
way.

—
Reply to this email directly or view it on GitHub
#4259 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Yes, plenty of times.

For example, the second most recent change to this same file was committed 19 days ago and had exactly 0 tests:
#4298

As this is more than a code style change, I feel that it needs testing.

That PR is a code style change. It changes nothing. Only makes it more readable. Yes code style PRs are merged directly.

This PR is CHANGING the installation SQL and it needs tests in different enviroments for example.

Also I'm not a big fan of insert ignore. It can lead you to expect that there something that is not really there. I prefer an installation error than not being able to be confident in the data you have. Yes the main id can be the same but the links between entities can be broken (assets, etc.)

Why are you changing it?

Agree with Matt - there is a MASSIVE difference between a code style commit
changing spaces and tabs and this,

On 8 October 2014 20:09, Matt Thomas notifications@github.com wrote:

As this is more than a code style change, I feel that it needs testing.

—
Reply to this email directly or view it on GitHub
#4259 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Sorry I'm on my smartphone. I just read the description.

For me it doesn't make sense. If you already need a recovery script why can't you add the logic to recover data there instead of restoring the full data?

Actually, #4298 wasn't just a codestyle change. It also changed the installation SQL:
https://github.com/joomla/joomla-cms/pull/4298/files#diff-135c6f58583408312a709459b19594c7R1499

Not only that, but there's plenty of other precedence on non-code-style changes.

Insert Ignore is just standard MySQL. It's supported all the way back to MySQL 4.1 (which neither Joomla 3 nor 2.5 support). For MySQL 5.1, it behaves exactly the same as 5.5:
http://dev.mysql.com/doc/refman/5.1/en/insert.html
http://dev.mysql.com/doc/refman/5.5/en/insert.html

As I understand it, there's practically 0 backward compatibility issues for a clean install of Joomla, especially due to MySQL's reliability.

It's as silly as testing for IF NOT EXISTS or ENGINE=InnoDB in #4298 (which didn't occur).

Anyway, it's not worth arguing about any further if it won't be accepted without testing. I'll just create some step-by-step testing instructions for it later today or this week.

@phproberto , it's for users that aren't capable of doing that. It's a lot easier for them to simply copy a file, replace the table prefix, and run it in phpMyAdmin. And it can be used as a general script for a variety of recovery issues.

As to asset issues, if I understand the issue properly, with the same data they are going to be there regardless whether or not the ignore statement is used. If for some reason there are asset issues, it would require someone with a relatively advanced skill set to troubleshoot it anyway.

I wouldn't recommend using the SQL files as recovery tools once you have any content in your database beyond the initial installation. The sample data (as is today) already proves that this is unreliable and can cause more issues than it's worth IMO. It's far too easy to screw up a site running any SQL commands directly against the database and re-running the installation SQLs will more than likely make a bad situation worse; not restore the initial system's integrity. I'm truthfully not trying to shoot down the idea, but from an administrator's standpoint, this really just opens the door for more problems.

No problem. I understand the reasoning. That makes sense in situations where there's an expert available to troubleshoot the issue. However, for most mom and pop sites, that's usually not the case.

Also, I'd say in most situations there wouldn't be any extra issues, since the install data is static and remains relatively constant. The main exception being discover installs.

That being said, the best solution ultimately is to fix the source of the problems (which is the updater) if possible. If a core extension is uninstalled, theoretically we shouldn't be reintroducing files for it (or run SQL for it, if that's happening). I'm not sure how to best do that though.

I think I'm going to go ahead and close this PR. The issue doesn't occur all that often, the "fix" is more of a bandaid, and it isn't automated nor is it well known. Not sure it's worth the hassle.

Thanks guys!