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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Aug 17, 2024
Merged

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

merged 6 commits into from
Aug 17, 2024

Conversation

vikiv480
Copy link
Contributor

@vikiv480 vikiv480 commented Aug 14, 2024 •
edited
Loading

  • 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

rexml/lib/rexml/parsers/baseparser.rb

Lines 560 to 563 in e14847c

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

Close #198

@vikiv480
Fix RuntimeError for valid feeds
981b424 
* Change `#entity` to not match against default entities

Close ruby#198
kou
kou reviewed Aug 15, 2024
View reviewed changes
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
@vikiv480 @kou
PR feedback
b5da207 
* Explicitly set `value` to `nil`
* Remove unnecessary conditional

Co-Authored-By: Sutou Kouhei <[email protected]>
@vikiv480
Add tests when default entities only
6555c27 
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.

@vikiv480 @naitoh
Improve readability for #entity
60632a5 
* 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

naitoh
naitoh reviewed Aug 16, 2024
View reviewed changes
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
@vikiv480 @naitoh
Remove unnecessary StringIO.new
b7e9378 
* 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
Reviewers

@naitoh naitoh naitoh left review comments

@kou kou kou left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@vikiv480 @kou @naitoh