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

Fix RuntimeError in REXML::Parsers::BaseParser for valid feeds #199

Merged
merged 6 commits into from
Aug 17, 2024

Conversation

vikiv480
Copy link
Contributor

@vikiv480 vikiv480 commented Aug 14, 2024

  • Change #entity to not match against default entities

After this change, the following example will not raise a RuntimeError:

# rexml/refactor_entity_example.rb

$LOAD_PATH.unshift(File.expand_path("lib"))

require "rexml/parsers/baseparser"

valid_feed = "<p>#{'A' * 10_240}</p>"

base_parser = REXML::Parsers::BaseParser.new("")
base_parser.unnormalize(valid_feed) # => "<p>" + "A" * 10_240 + "</p>"

Default entities now gets substituted by this block instead

else
er = DEFAULT_ENTITIES[entity_reference]
rv.gsub!( er[0], er[2] ) if er
end

Close #198

* Change `#entity` to not match against default entities

Close ruby#198
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this case?

lib/rexml/parsers/baseparser.rb Show resolved Hide resolved
lib/rexml/parsers/baseparser.rb Outdated Show resolved Hide resolved
@kou
Copy link
Member

kou commented Aug 15, 2024

@naitoh Could you review this?

vikiv480 and others added 2 commits August 15, 2024 21:44
* Explicitly set `value` to `nil`
* Remove unnecessary conditional

Co-Authored-By: Sutou Kouhei <[email protected]>
Adds tests for SAX parser, Pull parser and Stream parser when the source contains only default entities
@vikiv480
Copy link
Contributor Author

Could you add a test for this case?

I've added some tests in 6555c27, let me know what you think.

@naitoh
Copy link
Contributor

naitoh commented Aug 16, 2024

Could you review this?

before (This PR)

      def entity( reference, entities )
        value = nil
        value = entities[ reference ] if entities
        if value
          record_entity_expansion
          return unnormalize( value, entities )
        end

        nil
      end
  • If entities == nil, the return value is nil because value is always nil.
  • If value == nil, the return value is nil, so an early return is easier to understand.

after

      def entity( reference, entities )
        return unless entities

        value = entities[ reference ]
        return if value.nil?

        record_entity_expansion
        unnormalize( value, entities )
      end

Thanks.

* Early return if there is no `entities`
* Early return if there is no match for `reference` in `entities`

Co-Authored-By: NAITOH Jun <[email protected]>
@vikiv480
Copy link
Contributor Author

@naitoh Thanks for the suggestion, I like the change 👍 60632a5

Copy link
Contributor

@naitoh naitoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional review.

test/test_stream.rb Outdated Show resolved Hide resolved
* Remove unnecessary `StringIO.new`

Co-Authored-By: NAITOH Jun <[email protected]>
@naitoh
Copy link
Contributor

naitoh commented Aug 16, 2024

I have checked this PR.
I think this PR is good.
Thanks!

@kou kou merged commit 1c76dbb into ruby:master Aug 17, 2024
61 checks passed
@kou
Copy link
Member

kou commented Aug 17, 2024

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants