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

Handle the case of epmty msgid #45

Closed
wants to merge 1 commit into from

Conversation

loicfevrier
Copy link

When the msgid is multi-lines, I prefer not to have to do the implode in my code (no need to duplicate this line).
This commit allow a simple
setEntry(NULL, $entry)

and the msgid will be computed at the line 443 (new_msgid) which handle correctly the multi-line case.

Where the msgid is multi-line, I prefer not to have to do the implode in my code (no need to duplicate this line).
This commit allow a simple
setEntry(NULL, $entry)

and the msgid will be computed at the line 443 (new_msgid) which handle correctly the multi-line case.
@pherrymason
Copy link
Owner

Mmm I'm not very sure about making the first argument "optional".
Maybe the solution is to keep an internal custom identifier to reference each entry and then read everything from the $entry argument.

So when the entries are parsed, PoParsed adds a custom identifier to each entry, for example:

$entries = [
    ['__id' => 0,
     'msgid' => ['a msgid'],
     'msgstr' => ['translation']
    ],
    ['__id' => 1,
     'msgid' => ['another msgid', 'multilined']
     'msgstr' => ['translation', 'of a multiline string']
    ]
  ];

setEntry then will try to read this __id field to identify the entry being edited. If not found it means it is a new one.

Then developer user of the library doesn't have to handle with this details which I recognise it is not very confortable to work with.

@loicfevrier
Copy link
Author

Indeed.
Would you like me to developp it in that spirit or do you prefer to to it yourself ?

setEntry would become

setEntry($entry)

which would break compatibility.

@pherrymason
Copy link
Owner

You're very welcome to implement it! However I would add a change like that
in the 5.0 branch which already has that api, it only needs the internal id
El 03/06/2015 20:09, "Loïc Février" notifications@github.com escribió:

Indeed.
Would you like me to developp it in that spirit or do you prefer to to it
yourself ?

setEntry would become

setEntry($entry)

which would break compatibility.


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

@dereuromark
Copy link

Also

case (strpos($state, 'msgstr[') !== false):

needs to be a bit more defensive (in my case null for some reason):

case ($state && strpos($state, 'msgstr[') !== false):

Shouldnt throw warnings here:

Notice (8): Undefined index:  [ROOT/vendor/sepia/po-parser/src/Sepia/PoParser.php, line 307]

@pherrymason
Copy link
Owner

Feel free to create a PR for this modification

@pherrymason
Copy link
Owner

pherrymason commented Feb 2, 2018

This is no longer needed since #68
Thanks anyway!

#68 fixes your issue @dereuromark

@pherrymason pherrymason closed this Feb 2, 2018
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.

3 participants