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

No need to use YAML::Any when YAML::XS is required #2464

Closed
6 tasks done
bschmalhofer opened this issue Aug 15, 2023 · 3 comments
Closed
6 tasks done

No need to use YAML::Any when YAML::XS is required #2464

bschmalhofer opened this issue Aug 15, 2023 · 3 comments
Assignees
Labels
tidying Tidying of the code
Milestone

Comments

@bschmalhofer
Copy link
Contributor

bschmalhofer commented Aug 15, 2023

When looking at #2462 I noticed that there is a strangeness in which module is used by Kernel::System::YAML. Here is the present situation:
YAML::XSis required since OTOBO 10.0.0. Perl 5.24 was released in May 2016, so we can assume that YAML::LibYAML 0.62 is available. Kernel/cpan-lib/YAML.pm provides YAML 1.23which dates from February 2017. Judging from https://metacpan.org/release/TINITA/YAML-1.30/changes , this looks like several bugfixes to YAML are not included in the version used by OTOBO. Kernel::System::YAML uses Kernel/cpan-lib/YAML/Any.pm for selecting a YAML implementation. Effectively YAML::XS is picked always, as this is the preferred implementation. https://metacpan.org/dist/YAML/view/lib/YAML.pod recommends using YAML::XS.
_Kernel/cpan-lib/YAML,pmcan't be removed as it is required bySisimai`.

I propose:

  • keep Kernel/cpan-lib/YAML.pm and the dir Kernel/cpan-lib/YAML
  • require YAML::XS 0.62 in bin/otobo.CheckModules.pl
  • use YAML::XS directly in Kernel::System::YAML
  • remove the fallback to YAML because something is wrong anyways when YAML::XS can't load the data
  • adapt the singular test where the result changes
  • only test YAML::XS in scripts/test/YAML/YAML.t
@bschmalhofer bschmalhofer added the tidying Tidying of the code label Aug 15, 2023
@bschmalhofer bschmalhofer added this to the OTOBO 11.0 milestone Aug 15, 2023
@bschmalhofer bschmalhofer self-assigned this Aug 15, 2023
bschmalhofer added a commit that referenced this issue Aug 15, 2023
Because this was the current version when Perl 5.24 was released
bschmalhofer added a commit that referenced this issue Aug 15, 2023
Thus eliminate usage of YAML::Any and the fallback to YAML.pm.
Thus some code for special support of YAML.pm can be eliminated too.
bschmalhofer added a commit that referenced this issue Aug 15, 2023
The better semantics are decoding the string and then upgrading it.
bschmalhofer added a commit that referenced this issue Aug 15, 2023
Because this was the current version when Perl 5.24 was released
bschmalhofer added a commit that referenced this issue Aug 15, 2023
Thus eliminate usage of YAML::Any and the fallback to YAML.pm.
Thus some code for special support of YAML.pm can be eliminated too.
bschmalhofer added a commit that referenced this issue Aug 15, 2023
The better semantics are decoding the string and then upgrading it.
bschmalhofer added a commit that referenced this issue Aug 15, 2023
bschmalhofer added a commit that referenced this issue Aug 15, 2023
The changes for issue #2464 broke the script. The reason was that
the previous version used Kernel::System::YAML for testing
and the changes script, using YAML::XS, did not fully replicate the
funktionality in Kernel::System::YAML.
Furthermore there was some confusion regarding UTF8 de- and en-coding.
@bschmalhofer
Copy link
Contributor Author

Not closing the issue yet, as there are three failures related to FormDraft:

Test Summary Report
-------------------
/opt/otobo/scripts/test/Compile.t                                                                         (Wstat: 0 Tests: 1798 Failed: 0)
  TODO passed:   686, 811
/opt/otobo/scripts/test/Selenium/Agent/FormDraft/AgentTicketEmailOutboundDraft.t                          (Wstat: 256 (exited 1) Tests: 48 Failed: 1)
  Failed test:  48
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/Agent/FormDraft/AgentTicketForwardDraft.t                                (Wstat: 512 (exited 2) Tests: 50 Failed: 2)
  Failed tests:  49-50
  Non-zero exit status: 2
/opt/otobo/scripts/test/Selenium/Agent/FormDraft/AgentTicketPhoneCommonDraft.t                            (Wstat: 256 (exited 1) Tests: 47 Failed: 1)
  Failed test:  47
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/Output/Ticket/OverviewSmall.t                                            (Wstat: 256 (exited 1) Tests: 39 Failed: 1)
  Failed test:  39
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/TestingMethods.t                                                         (Wstat: 0 Tests: 36 Failed: 0)
  TODO passed:   21, 24, 28
Files=961, Tests=80406, 3701 wallclock secs (15.66 usr  2.18 sys + 700.23 cusr 97.21 csys = 815.28 CPU)
Result: FAIL
ran tests for product OTOBO 11.0.x on host 1858c59f4cdd .

@bschmalhofer
Copy link
Contributor Author

Test suite looks fine now. The three new failures were not related to YAML. Closing this issue.

@bschmalhofer
Copy link
Contributor Author

It would be nice to have a test case where multiple documents are contained in the input. This is an area where the different YAML implementations show differing behavior.
In scalar context, YAML::Syckand ỲAML::PPreturn the first document. YAML, YAML::Old, YAML::XS, und Cpanel::YAML::XS` return the last document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidying Tidying of the code
Projects
None yet
Development

No branches or pull requests

1 participant