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

Correcting parsing error since using : in Key #33

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Conversation

infograf768
Copy link
Owner

@infograf768 infograf768 commented Sep 24, 2021

We have new strings in com_admin.ini for 4.0.4 containing :
Image after patch
Screen Shot 2021-09-24 at 09 16 35

This works fine as : is authorized for parsing inis in general but creates errors in com_localise as : is not taken care of when parsing.
This PR adds the : in the translation model as well as in parseini.js

Before patch:
Screen Shot 2021-09-24 at 08 30 39

After patch
Screen Shot 2021-09-24 at 09 18 50

@Valc
This one is urgent

@Valc
Copy link
Contributor

Valc commented Sep 24, 2021

@infograf768
HEre is working fine. I think the new changes are under control with your patch.

@infograf768 infograf768 merged commit d9963de into master Sep 24, 2021
@infograf768 infograf768 deleted the parsing_error branch September 24, 2021 14:26
@infograf768
Copy link
Owner Author

Thanks for testing. Don't forget to update your branches. ;)

@Valc
Copy link
Contributor

Valc commented Sep 24, 2021

@infograf768
Due the issue now will crash the translation access on previour com_localise releases, i think we need a new com_localise release, please.

And, what about automaric updates? seems is not ready to get automatic alert about the new versions released from here.

@infograf768
Copy link
Owner Author

Making new pack now.
I hesitate to create an updateserver which would be placed in my repo as, normally, when we are ready to release, this should go to the original github repo which is a Joomla Project.

@infograf768
Copy link
Owner Author

@Valc
Copy link
Contributor

Valc commented Oct 8, 2021

@infograf768
At line 1993 of "LocaliseHelper.php" on "combineReferences" function also present the issue:

if (preg_match('/^([A-Z][A-Z0-9_\*\-\.]*)\s*=/', $line, $matches))

As changed with your patch should be:

if (preg_match('/^([A-Z][A-Z0-9_:\*\-\.]*)\s*=/', $line, $matches))

@Valc
Copy link
Contributor

Valc commented Oct 8, 2021

Making new pack now. I hesitate to create an updateserver which would be placed in my repo as, normally, when we are ready to release, this should go to the original github repo which is a Joomla Project.

Thanks, JM.

I have also seen that you have offered it where it corresponds, with no response at the moment.

joomla-projects/com_localise#340 (comment)

@infograf768
Copy link
Owner Author

@Valc
#33 (comment)

I can correct directly but it looks like the combineReferences method used only in the TranslationModel to get $this->item->source is useless.

@Valc
Copy link
Contributor

Valc commented Oct 8, 2021

I can correct directly but it looks like the combineReferences method used only in the TranslationModel to get $this->item->source is useless.

It certainly seems that this is used just to have that information in the item but in the end it is not used anywhere.

Anyway and since it is there, I think it does not hurt to fix it. Maybe in the future is required call that information from somewhere and the bug would already be solved.

If i am not wrong that info bypassed by that function was useful to create the translation language files with the right en-GB set of keys and strings (due maybe there was develop or not, or due we was using the local en-GB installed instance as reference or not).

But in this moment the struff creating the translation file based on the en-GB reference is working fine without to call "combineReferences"

If the bug is present, i suggest to solve it :)

@infograf768
Copy link
Owner Author

Corrected.

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.

2 participants