Mysql is OK but postgres not

The issue is actually coming from this line: https://github.com/joomla/joomla-cms/blob/staging/plugins/editors/codemirror/codemirror.php#L245 which has a wrong default value when retrieving the param.

No need to set the param during installation when it's fixed in that line.
However it would change the behavior for existing users. The other more B/C way would be to change the default value in the codemirror.xml so it matches the one in code.

I don't see how this would change any behavior at all for existing users.
The current solution is contained within the install sql - meaning no existing sites would ever be affected, and it is 100% B/c

out of general curiosity, why do we set half the params via the install, not set the other half the params, and then set them in the php file? that doesn't really make sense at all. It would make more sense (at least to me) to just set all default params on install, and then pull the settings from the database like everything else does.

I would need to check the commit history but I suspect that when these
params were added it was forgotten to update the SQL.
On 8 Aug 2015 4:43 pm, "Jon Neubauer" notifications@github.com wrote:

out of general curiosity, why do we set half the params via the install,
not set the other half the params, and then set them in the php file? that
doesn't really make sense at all. It would make more sense (at least to me)
to just set all default params on install, and then pull the settings from
the database like everything else does.

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

imo it would be better to take the defaults out of the php file, and set them on install via the install sql.
We shouldn't need to set a value in php when it's already set in the database.
thoughts?

The PHP checks have to stay in place. If you add new parameters, they don't get propagated into the database until someone saves an updated config for that item.

we're talking about the initial install, though - unless you're trying to say that not all of the install.sql file is actually placed into the database?

Updating the initial install is fine. You'll set the right defaults for new users. The PHP checks ensure the right behaviors are in place if those params aren't there for whatever reason (hacked up database, upgrades adding new params, etc.).

ahhh, ok, so those do need to stay regardless

... or do they? because the php file is calling the param by name, not by, say, position, which might be a problem if someone added new params that would change the order params are saved in.
maybe I'm thinking about this wrong, but someone adding a new param to a plugin wouldn't at all affect this?

The order isn't used, it's all referenced by name. The params are converted internally to a JRegistry object, so getting a param is the same as $registry->get($param, $default). If the param name isn't found, it uses whatever is specified in that $default param (defaults to null).

idk, still seems like setting the default isn't totally necessary.
but either way, no problem, I still think we should set the default settings in the db on install, as this PR does.

Getting it right on install is important, yes. But the PHP checks have to stay in place; we do not re-save every extension's configuration on an update, so if an extension adds a new parameter, that value isn't stored to the database until the user saves the extension with an updated configuration.

Not setting a default in PHP becomes an issue if the code is working with expected known values. If you have a select list with 4 options and set a switch case on the possible values of that select list without a default case, then something in that extension can break. Either using a default case or having a default return value for the param if it isn't found in the data store gets past this.

that makes more sense, thanks for the explanation

Actually, the parameters in the install SQL are imho useless if they are the same as the default values (which they should), but that's me

The defaults in code MUST be the same as the ones defined in the XML. Otherwise you get that funny effect that something acts different after you just open and saved the parameters without changing anything. And this is the reason why it has to be changed in code or xml anyway. Because currently it will highlight the line until you save the params and then suddenly stops without the user actually changing a param.
Your PR wouldn't solve that for existing users, only for new ones.

Personally I would change the XML file to have default="1" (because that's actually what the behavior is currently) and also change the PHP code from $this->params->get('activeLine', true); to $this->params->get('activeLine', 1); because the default value actually is 1 and not true.

You can add the params to the SQL files, but it isn't really needed. All 3rd party extensions work fine without this anyway

why are we trying to set activeLine to true? CodeMirror itself, by default (non-Joomla), does not set activeLine to true - this is something someone (my guess is by mistake) set in the PHP file that is causing confusion.

In my opinion it should be set to 0, if we're going to be setting it to anything.

I've already noted at least a couple times that this is only a fix for new users, as well as the reason why a fix for only new users is most likely appropriate.

Yeah I know you only want to fix it for new users. But the real issue is for existing sites where the behavior suddenly changes after they saved the params. This is a confusing thing and the cause is a real bug in the code. Which should be fixed. Default values in XML and PHP have to match.

I don't care much which way we do it. Setting it to 1 is full backward compatible as nothing changes for existing users. Setting it to 0 is a small backward compatibility break, but imho one we can do as nothing really breaks. It's just a visual thing.

Setting it only in the installation SQL is basically only a workaround without fixing the bug.

I'll update it to make the XML and php files match, then ????

On Sat, Aug 8, 2015, 4:50 PM Thomas Hunziker notifications@github.com
wrote:

Yeah I know you only want to fix it for new users. But the real issue is
for existing sites where the behavior suddenly changes after they saved the
params. This is a confusing thing and the cause is a real bug in the code.
Which should be fixed. Default values in XML and PHP have to match.

I don't care much which way we do it. Setting it to 1 is full backward
compatible as nothing changes for existing users. Setting it to 0 is a
small backward compatibility break, but imho one we can do as nothing
really breaks. It's just a visual thing.

Setting it only in the installation SQL is basically only a workaround
without fixing the bug.

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

Great

Setting to Needs Review. @jneubauer is unlikely to update/complete this PR so either someone needs to take it over or it can be closed.

NOTE in the original comment it says the active line colour is the same as the highlited text colour. Not sure if something chaged on codemirror but they are different if only subtly

Closing as this PR is incomplete. Thank you all for your contribution. It will be looked at by @n9iels

As @brianteeman mentioned, there is a clear difference between a selected and en active line now. See the screenshots below:

Active line

Selected line

Also I checked the code and there is a fallback for each parameter like $this->params->get('activeLine', true). The problem is that this parameter is not set in the database and the fallback value is true, but the default value in the XML file is false. This results in a highlighted active line on first use, but when open and save the plugin the active line is no longer highlighted.

I will create a PR that sets the default values in the XML file equal to the fallback values in the PHP file.