8000 [DomCrawler] Added argument `$default` to method `Crawler::attr()` by Rastishka · Pull Request #51368 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DomCrawler] Added argument $default to method Crawler::attr() #51368

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
Merged
merged 1 commit into from
Aug 16, 2023
Merged

[DomCrawler] Added argument $default to method Crawler::attr() #51368

merged 1 commit into from
Aug 16, 2023

Conversation

Rastishka
Copy link
Contributor
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets no
License MIT
Doc PR symfony/symfony-docs#...

Added argument $default to method ->attr() like ->text() and ->html() have.

@carsonbot carsonbot added this to the 6.4 milestone Aug 12, 2023
@nicolas-grekas
Copy link
Member

Do we really need this? My feeling is that we now have the ?? operator and it's nicer to use.

@antonkomarev
Copy link
antonkomarev commented Aug 12, 2023 •
edited
Loading

I'd prefer to use native ??. It feels more obvious about the result.

@Rastishka
Copy link
Contributor Author

Do we really need this? My feeling is that we now have the ?? operator and it's nicer to use.

Yes we need, because setting default value also prevents throwing exception when node does not exist (methods text() and html() act similar).

So this code:

try {
    $value = $dom->filter('a.someclass')->attr('href');
} catch (\InvalidArgumentException $e) {
    $value = '';
}

can be simplifies to
$value = $dom->filter('a.someclass')->attr('href', '');

nicolas-grekas
nicolas-grekas reviewed Aug 14, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. Works for me.

@nicolas-grekas
Copy link
Member

Please also update the UPGRADE-6.4.md file, and please rebase to get rid of merge commits (PRs shouldn't contain any).

@nicolas-grekas nicolas-grekas changed the title Added argument $default to method ->attr() like ->text() and ->html() have. Added argument $default to method Crawler::attr() Aug 16, 2023
@carsonbot carsonbot changed the title Added argument $default to method Crawler::attr() [DomCrawler] Added argument $default to method Crawler::attr() Aug 16, 2023
@nicolas-grekas
Copy link
Member

Thank you @Rastishka.

@nicolas-grekas nicolas-grekas merged commit c2ac73c into symfony:6.4 Aug 16, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 16, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
DomCrawler Feature Status: Needs Review
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
4 participants
36D0 @Rastishka @nicolas-grekas @antonkomarev @carsonbot
0