fabriciocolombo / delphi-rest-client-api

A Delphi REST client API to consume REST services written in any programming language.
Apache License 2.0
380 stars 182 forks source link

UTF8/Unicode issue with < Delphi 2009 #38

Open ghost opened 9 years ago

ghost commented 9 years ago

Bugreport: Wrong function definition leads to a loss of widestring information.

function Get: string;overload;
begin
    ...
    Result := UTF8Decode(...);
end;

UTF8Decode returns a widestring, function returns a string. All non charset-character-information is lost. This error occurs on following functions:

function Get: string;overload;
function Post(Content: TStream): string;overload;
function Post(Content: string): string;overload;
function Put(Content: TStream): string;overload;
function Put(Content: string): string;overload;
function Patch(Content: TStream): string;overload;
function Patch(Content: string): string;overload;

I've temporarily changed the return type from string to widestring to solve this issue.

RobertoSchneiders commented 9 years ago

Thanks for the report.

String means different things between different versions of Delphi, so, is a mess.

I am not so sure that the widestring is the right type here (for all delphi versions). Maybe is better to use compiler directives in this case.

What do you think?

ghost commented 9 years ago

Yes I think so, too.

I think that type string became a unicode string type by default since Delphi 2009. If you can confirm this I could add compiler directives.

option 1:

function TResource.Get: {$IFDEF DELPHI_2009_UP}string{$ELSE}Widestring{$ENDIF};
begin
  Result := FRestClient.DoRequest(METHOD_GET, Self);
end;

option 2:

{$IFDEF DELPHI_2009_UP}
function TResource.Get: String
{$ELSE}
function TResource.Get: Widestring
{$ENDIF};
begin
  Result := FRestClient.DoRequest(METHOD_GET, Self);
end;

option 3:

{$IFDEF DELPHI_2009_UP}function TResource.Get: String{$ELSE}function TResource.Get: Widestring{$ENDIF};
begin
  Result := FRestClient.DoRequest(METHOD_GET, Self);
end;

1.) Should I sent you a pull request? 2.) Which option do you prefer?

RobertoSchneiders commented 9 years ago

I'm not sure about that, but I guess that string is a widestring type in some 2009+ versions of delphi, and it is UnicodeString in the latest versions. But it doesn't matter. Your solution will solve the problem anyway.

  1. Absolutely. You can write a test for this problem?
  2. I prefer the first option, less code duplication. I hope we have more opinions from the other members. :)
ronaldhoek commented 9 years ago

Why not define the string type using defines and use it so there's no need for the defines inside the class. This way you can use it for all other routines too!!!

type
{$IFDEF DELPHI_2009_UP}
RestString = String
{$ELSE}
RestString = WideString
{$ENDIF}

TResource = class(...)
...
  function Get: RestString;
...
end;

function TResource.Get: RestString
begin
  Result := FRestClient.DoRequest(METHOD_GET, Self);
end;
RobertoSchneiders commented 9 years ago

much better @ronaldhoek :+1:

ghost commented 9 years ago

@ronaldhoek: very nice solution!

@RobertoSchneiders: I'll do it in the way @ronaldhoek suggested. You'll hearing from me soon.

fabriciocolombo commented 9 years ago

@ronaldhoek :+1:

SuperObject use the way below, then don't depend of a specific compiler directive or be aware at which delphi version includes unicode support.

{$if (sizeof(Char) = 1)}
  RestString = WideString;
{$else}
  RestString = string;
{$ifend}
ronaldhoek commented 9 years ago

Ah, sorry about that - it was just a copy-paste from some unit I knew it was in...

BTW: starting with the unicode versions of Delphi, the define 'UICODE' was introduced. So you can also use {$IFDEF UNICODE} instead off {$if (sizeof(Char) = 1)}

fabriciocolombo commented 9 years ago

Looks better use {$IFDEF UNICODE}