Issue 29355234: Issue 4293 - Make Textpattern use our current website content as template (Closed)
|
Created:
Sept. 28, 2016, 10:02 a.m. by Wladimir Palant Modified:
Sept. 28, 2016, 2:46 p.m. Reviewers:
kzar CC:
saroyanm, juliandoucette Visibility:
Public. |
DescriptionThis is a Textpattern plugin, not tracked in any repository. A cron job will download https://adblockplus.org/en/documentation regularly and save it as site_template file.
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed nit #Patch Set 3 : Don't remove hamburger menu #MessagesTotal messages: 10
https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode1 plugin.php:1: $template_data = FALSE; Nit: I guess this should be NULL not FALSE? https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); We don't check for the closing `>` after "hamburger"?
Added Julian to CC on this review.
https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode1 plugin.php:1: $template_data = FALSE; On 2016/09/28 10:11:07, kzar wrote: > Nit: I guess this should be NULL not FALSE? Done. https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); On 2016/09/28 10:11:07, kzar wrote: > We don't check for the closing `>` after "hamburger"? Do we care about it? It is certainly there somewhere but we skip until the closing </a>.
https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); On 2016/09/28 10:14:42, Wladimir Palant wrote: > On 2016/09/28 10:11:07, kzar wrote: > > We don't check for the closing `>` after "hamburger"? > > Do we care about it? It is certainly there somewhere but we skip until the > closing </a>. I guess you're right it doesn't matter too much, but in that case it should be `.*` instead of `.*?` since there should always be at least one character after "hambuger".
https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); On 2016/09/28 10:19:11, kzar wrote: > On 2016/09/28 10:14:42, Wladimir Palant wrote: > > On 2016/09/28 10:11:07, kzar wrote: > > > We don't check for the closing `>` after "hamburger"? > > > > Do we care about it? It is certainly there somewhere but we skip until the > > closing </a>. > > I guess you're right it doesn't matter too much, but in that case it should be > `.*` instead of `.*?` since there should always be at least one character after > "hambuger". Argh, I meant `.+?`...
https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); On 2016/09/28 10:19:11, kzar wrote: > I guess you're right it doesn't matter too much, but in that case it should be > `.*` instead of `.*?` since there should always be at least one character after > "hambuger". You probably mean .+? (still a non-greedy regular expression but one that requires at least one character). The input is our own code and we don't need to validate it so the regular expressions are somewhat sloppy in order to be simpler. I definitely prefer "skip some number of characters without caring how many" - this way you don't need to check whether your assumption about the number of characters is true.
LGTM https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php File plugin.php (right): https://codereview.adblockplus.org/29355234/diff/29355235/plugin.php#newcode19 plugin.php:19: $header = preg_replace('/<a\b[^>]*\bid="hamburger".*?<\\/a>/s', '', $header); On 2016/09/28 10:32:48, Wladimir Palant wrote: > On 2016/09/28 10:19:11, kzar wrote: > > I guess you're right it doesn't matter too much, but in that case it should be > > `.*` instead of `.*?` since there should always be at least one character > after > > "hambuger". > > You probably mean .+? (still a non-greedy regular expression but one that > requires at least one character). The input is our own code and we don't need to > validate it so the regular expressions are somewhat sloppy in order to be > simpler. I definitely prefer "skip some number of characters without caring how > many" - this way you don't need to check whether your assumption about the > number of characters is true. Yep that's what I meant, sorry. Makes sense anyway, I guess there's no point over-complicating it.
One more changeset - I realized that the hamburger menu should be present on the blog as well.
LGTM 
|
