?

Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
20 Apr 2021

Pull Request for Issue #31160 joomla/cassiopeia#185

Summary of Changes

Disable language debugging before inserting translated sample data (blog or testing).

Testing Instructions

Enable language debugging on a fresh installation, then install sample data (blog or testing).

Actual result BEFORE applying this Pull Request

Where ** = debug_lang_const

Titles and texts have surrounding ** from language debugging, and these values are stored into the db by mistake

Expected result AFTER applying this Pull Request

Sample/Testing data installed with no hard coded debug_lang_const / ** value saved to the db

Documentation Changes Required

none

// @chmst @infograf768

avatar PhilETaylor PhilETaylor - open - 20 Apr 2021
avatar PhilETaylor PhilETaylor - change - 20 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Apr 2021
Category Front End Plugins
avatar PhilETaylor PhilETaylor - change - 20 Apr 2021
Labels Added: ?
avatar richard67
richard67 - comment - 21 Apr 2021

I would expect the debug to be switched on again after having written to db if it was on before switching off, but I don't see that in the code changes. Is that already happening somewhere else so it doesn't need to be added? Or am I thinking wrong in some way?

avatar Fedik
Fedik - comment - 21 Apr 2021

The fix does not looks right to me.
I did not seen that issue before, but the purpose of enabling debug is to debug, not for disable it "just here" :)

Also the debug set when Language initialized, so changing App debug does not affect it after Language initialized.

*/
public function __construct($lang = null, $debug = false)
{
$this->strings = array();
if ($lang == null)
{
$lang = $this->default;
}

I would just suggest to disable debug manually, when debugging does not need.

avatar infograf768
infograf768 - comment - 21 Apr 2021

Agree with @Fedik
This is not needed.

avatar PhilETaylor PhilETaylor - change - 21 Apr 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-04-21 09:01:45
Closed_By PhilETaylor
avatar PhilETaylor PhilETaylor - close - 21 Apr 2021
avatar brianteeman
brianteeman - comment - 21 Apr 2021

I would just suggest to disable debug manually, when debugging does not need.

That is ok if you know about this problem but if you dont then it appears that Joomla is broken AND as you cant remove sample data without completely reinstalling Joomla then it definitely should be resolved.

The methodology proposed by @PhilETaylor is correct and we use it already in most core views for EXACTLY this reason - it was introduced 10 years ago by @infograf768

@richard67 is also correct that after turning it off you also need to turn it back on again

THINK LIKE A USER NOT A DEVELOPER

avatar PhilETaylor
PhilETaylor - comment - 21 Apr 2021

so changing App debug does not affect it after Language initialized.

Proof you never even bother testing - because this PR fixes the reported issue and language debugging can be changed after init.

As you have both rejected this PR please go and close the open issue with a 'won't fix' also

avatar brianteeman
brianteeman - comment - 21 Apr 2021

@PhilETaylor I see while I was typing my response that you closed this. Please re-open it this absolutely needs to be fixed.

avatar richard67
richard67 - comment - 21 Apr 2021

@PhilETaylor Please re-open. I can understand that you don't wanna be bothered with endless discussions, but sometimes a bit discussion is needed, and it needs to convince people sometimes.

avatar PhilETaylor
PhilETaylor - comment - 21 Apr 2021

I would expect the debug to be switched on again after having written to db if it was on before switching off,

After data is written to the db the request ends and the response is returned to the browser

The setting is only changed in memory, this PR doesn't update configuration.php and so on the next and also parallel requests, debug is still on.

As the response is JSON, having debug turned back on after writing to the db serves no purpose, as the request has already terminated before you blink

avatar rdeutz rdeutz - change - 21 Apr 2021
Status Closed New
Closed_Date 2021-04-21 09:01:45
Closed_By PhilETaylor
avatar rdeutz rdeutz - change - 21 Apr 2021
Status New Pending
avatar rdeutz rdeutz - reopen - 21 Apr 2021
avatar PhilETaylor
PhilETaylor - comment - 21 Apr 2021

@brianteeman I don't even have GitHub permissions to reopen my own issues...

avatar richard67
richard67 - comment - 21 Apr 2021

I would expect the debug to be switched on again after having written to db if it was on before switching off,

After data is written to the db the request ends and the response is returned to the browser

The setting is only changed in memory, this PR doesn't update configuration.php and so on the next and also parallel requests, debug is still on.

As the response is JSON, having debug turned back on after writing to the db serves no purpose, as the request has already terminated before you blink

That makes sense to me. I was asking because that was not clear to me.

avatar Fedik
Fedik - comment - 21 Apr 2021

That is ok if you know about this problem but if you dont then it appears that Joomla is broken AND as you cant remove sample data without completely reinstalling Joomla then it definitely should be resolved.

This is "hammer lesson": if you have hit your finger once, then next time you will be more careful 😉

The option debug_lang is Off by default. If User enabling it then he/she should know what he/she doing.

avatar brianteeman
brianteeman - comment - 21 Apr 2021

I prefer not to have a hammer

avatar brianteeman
brianteeman - comment - 21 Apr 2021

or to wear a safety glove so that if I miss the nail and hit my finger it wont hurt

avatar PhilETaylor
PhilETaylor - comment - 21 Apr 2021

Im finally now out of bed - Im officially off work with back pain/spasms.

I KNOW that this PR looks wrong, like some other of my PRs, I try to tackle the bigger issues and are more complex and therefore need complex, or controversial changes. I see its had a shark attack also, and I can see both sides of the argument.

I closed it, as there is no point flogging a dead horse. If two maintainers, or two senior advisors/trusted people disagree then there is no point flogging a PR - and leaving it unresolved without merge or final decision for the next 5 years.. this is how this repo gets flooded with wood so the trees cannot be seen.

There is a genuine reason to allow the ** into the db - the reason for this would be to debug if the sample data language placeholders were being translated or not (the whole reason for language debug in the first place)

But equally, that feature (the ability to see which sample data is and is not translated when inserted to the db) is used by less than 10 people globally, mostly the people in this thread already, and only normally in the development of Joomla core, in this GitHub repo.

A "Normal USER" is using language debug for other reasons, for developing their SITE, not for designing the platform (Joomla) that their site is running.

The only bug in this PR is that the success/error messages, do not get surrounded in debug_lang_const/** - that alone might be a reason to reenable the debug before returning the response, or for rejecting this PR outright.

Alternatives to this PR were already considered, and personally rejected, but might be more palatable to others, including but not limited to

  • Adding an additional param to Text::_() method signature of $debug = false which would overwrite the class property for the debug, but only for that one call of _() method - rejected due to signature changing, and performance overhead as this method is called A LOT on a page load.
  • Adding a new method to Text class to remove the debug_lang_const/** surrounding already translated text, and then purposing that new method on the translated sample data
  • Adding a single Factory::getLanguage()->setDebug(false); to the constructor of the plugins (but this has a knock on effect as these plugins are init to draw the admin module) - could check if its an ajax request before setting debug to false in the constructor though,
avatar Fedik
Fedik - comment - 21 Apr 2021

I have wrote my concern about the changes, no more no less.

The main point: User should not use debug if he does not need to debug.

I can suggest to warn User before executing the SampleData script:

var msg = 'The debug_lang is enabled, this will debug translation strings for the Samle data, are you know what you doing?';
if (debug_lang_enabled && !confirm(msg)){
  // stop everything
}

With a better message, of course.
That will be a safety glove that @brianteeman suggested to use :)

avatar dgrammatiko
dgrammatiko - comment - 21 Apr 2021

Enable language debugging on a fresh installation, then install sample data (blog or testing).

That is another architectural flaw of the sampledata. AFAIK there is no core field that is storing translatable strings in the DB. Also since the problem is sampledata just fix it there (eg on the ajax functions of the plugins as you are doing) so this should count as an acceptable solution.

BTW, to add on the flaws of sampledata, disable tags and versions and check what happens if you try to install the sampledata...

avatar PhilETaylor
PhilETaylor - comment - 21 Apr 2021

So is everyone in agreement that this PR is dead in favour of a Javascript punch in the face if language debug is enabled, to allow a user to stop and think about their life before they proceed?

avatar dgrammatiko
dgrammatiko - comment - 21 Apr 2021

So is everyone in agreement that this PR is dead in favour of a Javascript punch in the face if language debug is enabled, to allow a user to stop and think about their life before they proceed?

I'm not, there are many flaws with the sampledata and JS can only hide them under the rag...

avatar richard67
richard67 - comment - 21 Apr 2021

Are you talking about Blog Sample Data, or Testing Sample Data, or both?

avatar dgrammatiko
dgrammatiko - comment - 21 Apr 2021

Are you talking about Blog Sample Data, or Testing Sample Data, or both?

All of them. There are 0 checks that the extensions needed are even enabled, that should be the first step of the logic
I've pointed before on my implementation which is not feature complete but a way better starting point: https://github.com/dgrammatiko/sloth-pkg/blob/main/plg_sampledata/sloth.php

Edit and it doesn't have an issue with debug...

avatar chmst
chmst - comment - 23 Apr 2021

@dgrammatiko There are checks in blog sample data if tags, fields and workflow are enabled. No check for com_content as this cannot be disabed.


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

avatar dgrammatiko
dgrammatiko - comment - 23 Apr 2021

@dgrammatiko There are checks in blog sample data if tags, fields and workflow are enabled

There might be checks but you're missing the point, the sampledata is all about restoring the state of a Joomla instance (eg db data + file data) so having something like

if (!JComponentHelper::isEnabled('com_modules') || !Factory::getUser()->authorise('core.create', 'com_modules'))
{
$response = array();
$response['success'] = true;
$response['message'] = JText::sprintf('PLG_SAMPLEDATA_BLOG_STEP_SKIPPED', 3, 'com_modules');
return $response;
}
doesn't really help people (in case they played around with their installation and disabled things). In sort what the code has is error checking but no code to restore the state to the expected one. In sort, sampledata assumes that it will run on an untouched installation or there is no guaranty that the outcome will be the expected one (try disabling some front end modules before applying sampledata). Here's what I get if i disable all the modules except menu:
Screenshot 2021-04-23 at 11 20 38

Screenshot 2021-04-23 at 11 20 18

Screenshot 2021-04-23 at 11 20 50

And sorry but that is not my expectation, I expect the sampledata to restore correctly the state to what should be the demo...

avatar infograf768
infograf768 - comment - 24 Apr 2021

I expect the sampledata to restore correctly the state to what should be the demo...

That would become tricky as we can install multilang sample data and then blog sample data for each admin language.

avatar Bakual
Bakual - comment - 24 Apr 2021

There might be checks but you're missing the point, the sampledata is all about restoring the state of a Joomla instance (eg db data + file data) so having something like

No, Sample Data explicitely doesn't expect to run on an clean installation. And no, it is isn't expected to enable disabled components. That's the reason such checks are included. I can say that cause I designed it 😄
That's also why complete steps can be skipped.
If a check is missing, then it has to be added.

No check for com_content as this cannot be disabed.

That's not exactly true 😄 :

if (!JComponentHelper::isEnabled('com_content') || !Factory::getUser()->authorise('core.create', 'com_content'))

As for this PR itself, I don't really have an opinion :-)

avatar dgrammatiko
dgrammatiko - comment - 24 Apr 2021

And no, it is isn't expected to enable disabled components. That's the reason such checks are included. I can say that cause I designed it 😄

The code will just fail if something is disabled. Why is this acceptable since the code can just have an array of all the extensions needed and enabling them if they're disabled in order to restore the state correctly. Throwing a message with some cryptic text that tells the user to do some operation (that can be done automatically) is not exactly the UX I would expect from any software in 2021.

That would become tricky as we can install multilang sample data and then blog sample data for each admin language.

If the current sampledata cannot do that (eg restore correctly some db data and any files required) is a good indication that it needs some love.

The point here is that this API (sampledata) was supposed to eliminate the need for template (or other extensions) to roll their own installation for their demos and checking the adoption it seems it totally missed the target. There is no way to snapshot an installation and create the sampledata plugin, everything needs to be done painstakingly by hand. That's orders of magnitude more inefficient than exporting the db data and replacing the installation sql files. There were good intentions but unfortunately, the implementation is not really helpful for the devs, too much boilerplate to do what should be a walk in the park...

avatar Bakual
Bakual - comment - 24 Apr 2021

The code will just fail if something is disabled. Why is this acceptable since the code can just have an array of all the extensions needed and enabling them if they're disabled in order to restore the state correctly. Throwing a message with some cryptic text that tells the user to do some operation (that can be done automatically) is not exactly the UX I would expect from any software in 2021.

Because it is expected to skip, not fail. If it fails, then it is a bug which should be fixed.
Sample data can be for multiple extensions. Eg testing data could contain data for com_weblinks which isn't even installed. Thus it has to check and skip if not available.

The point here is that this API (sampledata) was supposed to eliminate the need for template (or other extensions) to roll their own installation for their demos and checking the adoption it seems it totally missed the target.

No, that wasn't the initial goal. The main goal was to get rid of unmaintainable core sampledatas which at that time was more or less an SQL dump. It was a nice side effect that it allows other extensions (like templates) to do their sample data as well.
What those extensions do in their plugins is of course up to them. You could do an SQL dump and restore that. You could break the whole site if you wanted. Nothing in the code is stopping you here. But yeah, you need to write that code yourself.

avatar PhilETaylor
PhilETaylor - comment - 24 Apr 2021

This PR has descended into a general conversation about the merits and failures of the sample data feature, rather than about the specific issue that was reported that this PR attempts to resolve.

Either this receives two successful tests and is merged, or it is rejected outright. There is no point in leaving this open unless we steer towards one or the other of those outcomes, lest we have yet another stagnant PR, That is left open for years with no action

avatar infograf768
infograf768 - comment - 24 Apr 2021

That would become tricky as we can install multilang sample data and then blog sample data for each admin language.

If the current sampledata cannot do that (eg restore correctly some db data and any files required) is a good indication that it needs some love.

You did not get it. It is extremely useful to first install multilang sample and then blog sample per lang.

avatar dgrammatiko
dgrammatiko - comment - 24 Apr 2021

You did not get it. It is extremely useful to first install multilang sample and then blog sample per lang.

I think you don't get what I'm saying. The process should be intuitive, eg hit one button and not install multilingual sampledata then for each language the sample blog. It's 2021 and we have all the data in hand to do this in one step instead of asking people to have a PHD to restore a demo state. But then again we're talking about Joomla and everything needs to be extremely complicated for no logical reason...

avatar Bakual
Bakual - comment - 24 Apr 2021

Please open a new issue if you think Sample Data needs to be improved. Phil is right that this is not the right place to discuss this.

avatar dgrammatiko
dgrammatiko - comment - 24 Apr 2021

Please open a new issue if you think Sample Data needs to be improved. Phil is right that this is not the right place to discuss this.

There is already an issue: #32878

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

@richard67 please can you pop another $1 in the meter for https://ci.appveyor.com/project/release-joomla/joomla-cms/builds/38803622/job/fs6svs4ciij6r5vw to get it to work. I dont like the red cross ;-)

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

Turning off debug mode - doing something and then turning it back on again has precedent in Joomla in 10 other places - mainly in toolbars

Screenshot 2021-04-27 at 18 59 17

avatar infograf768
infograf768 - comment - 28 Apr 2021

This code is only used for 3rd party extensions which have a custom help url. It concerns only the url itself which should never be debugged.

For core we use $ref_key or $help->key for example
JHELP_COMPONENTS_COM_CONTENT_OPTIONS="Articles:_Options" which never should be translated in language packs as it represents part of a keyref
and
$url in this case is NULL

It gives

<button class="button-help btn btn-info" type="button" onclick="Joomla.popupWindow('https:\/\/help.joomla.org\/proxy?keyref=Help40:Articles:_Options&amp;lang=en', '**Help**', 700, 500, 1)" title="**Opens in a new window**">
	<span class="icon-question" aria-hidden="true"></span>
	**Help**</button>

But for a third party component the string would be, if one creates a $lang_help_url
COM_MYCOMPONENT_HELP_URL="whatever"

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

So Im at a loss what I need to do next to get this over the line to make the most amount of people happy...

Is there any middle ground you lot can agree on?

If not then I guess we close this here and move on.

avatar PhilETaylor PhilETaylor - change - 2 May 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-05-02 15:25:43
Closed_By PhilETaylor
avatar PhilETaylor PhilETaylor - close - 2 May 2021

Add a Comment

Login with GitHub to post a comment