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

Image block: clean-up and improve usage of WP_HTML_Tag_Processor #55123

Closed
afercia opened this issue Oct 6, 2023 · 1 comment · May be fixed by WordPress/wordpress-develop#5428
Closed

Image block: clean-up and improve usage of WP_HTML_Tag_Processor #55123

afercia opened this issue Oct 6, 2023 · 1 comment · May be fixed by WordPress/wordpress-develop#5428
Assignees
@artemiomorales @dmsnell
Labels
[Block] Image Affects the Image Block [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts

Comments

@afercia
Copy link
Contributor

afercia commented Oct 6, 2023

Description

Splitting this out from #55010

See comment #55010 (comment)

Turns out there's room for improvements and clean-up the code of the Image block, specifically:

  • reduce the amounc of WP_HTML_Tag_Processor being created
  • better variable names: avoid things like $w, $p and the iike
  • consider using WP_HTML_Tag_Processor bookmarks

Step-by-step reproduction instructions

This is a code quality / performance issue. No bugs to reproduce.
As aknowledged in #55010 (comment) the Block image PHP part has room for improvements as some code is redundant / not necessary and duplicate unnecessary instances of WP_HTML_Tag_Processor are still in place from previous implementations.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes
@afercia afercia added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality [Block] Image Affects the Image Block labels Oct 6, 2023
@artemiomorales artemiomorales mentioned this issue Oct 6, 2023
Open
69 tasks
dmsnell added a commit to dmsnell/wordpress-develop that referenced this issue Oct 7, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
c4670cd 
Resolves WordPress/gutenberg#55123

Replaces existing single-letter-named Tag Processor instances with
a single processor that's reuse throughout the function.

As a work-in-progress (WIP) patch, this is a preliminary exploration
and not ready for review or merging.
@dmsnell dmsnell mentioned this issue Oct 8, 2023
Draft
@dmsnell dmsnell mentioned this issue Oct 11, 2023
Closed
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 11, 2023
dmsnell added a commit that referenced this issue Oct 11, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
fdb0b2f 
Companion work in WordPress/wordpress-develop#5428

Resolves #55123

This patch refactors the use of the HTML API in image lightbox rendering.
It replaces creating multiple Tag Processor instances with the use of a
single instance that rewinds and jumps around to perform multiple jobs
on a single instance.

Also incorporated is a fix for bailing out when no images are in a given
image block's output HTML.
dmsnell added a commit to dmsnell/wordpress-develop that referenced this issue Oct 12, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
064a71f 
Resolves WordPress/gutenberg#55123

Replaces existing single-letter-named Tag Processor instances with
a single processor that's reuse throughout the function.

As a work-in-progress (WIP) patch, this is a preliminary exploration
and not ready for review or merging.
dmsnell added a commit to dmsnell/wordpress-develop that referenced this issue Oct 12, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
f924e03 
Resolves WordPress/gutenberg#55123

Replaces existing single-letter-named Tag Processor instances with
a single processor that's reuse throughout the function.

As a work-in-progress (WIP) patch, this is a preliminary exploration
and not ready for review or merging.
dmsnell added a commit to dmsnell/wordpress-develop that referenced this issue Oct 12, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
fcb9061 
Resolves WordPress/gutenberg#55123

Replaces existing single-letter-named Tag Processor instances with
a single processor that's reuse throughout the function.

As a work-in-progress (WIP) patch, this is a preliminary exploration
and not ready for review or merging.
dmsnell added a commit to dmsnell/wordpress-develop that referenced this issue Oct 12, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
e47a3d0 
Resolves WordPress/gutenberg#55123

Replaces existing single-letter-named Tag Processor instances with
a single processor that's reuse throughout the function.

As a work-in-progress (WIP) patch, this is a preliminary exploration
and not ready for review or merging.
dmsnell added a commit that referenced this issue Oct 15, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance.
9d56eb8 
Companion work in WordPress/wordpress-develop#5428

Resolves #55123

This patch refactors the use of the HTML API in image lightbox rendering.
It replaces creating multiple Tag Processor instances with the use of a
single instance that rewinds and jumps around to perform multiple jobs
on a single instance.

Also incorporated is a fix for bailing out when no images are in a given
image block's output HTML.
dmsnell added a commit that referenced this issue Oct 16, 2023
@dmsnell
Lightbox: Re-use existing Tag Processor instance. (#55281)
7b3ec58 
Companion work in WordPress/wordpress-develop#5428

Resolves #55123

This patch refactors the use of the HTML API in image lightbox rendering.
It replaces creating multiple Tag Processor instances with the use of a
single instance that rewinds and jumps around to perform multiple jobs
on a single instance.

Also incorporated is a fix for bailing out when no images are in a given
image block's output HTML.
@michalczaplinski
Copy link
Contributor

Closing this now as the PHP side of the Image block went through some refactoring since. It uses bookmarks now when possible and avoids re-creating the Tag Processor instances.

Feel free to open new issues for additional improvements or send a PR directly!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
4 participants
@afercia @artemiomorales @michalczaplinski @dmsnell