') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Rebuilding Format class by mostafakhudair · Pull Request #3149 · codeigniter4/CodeIgniter4 · GitHub
Skip to content

Rebuilding Format class - #3149

Closed
mostafakhudair wants to merge 16 commits into
codeigniter4:developfrom
mostafakhudair:patch-12
Closed

Rebuilding Format class#3149
mostafakhudair wants to merge 16 commits into
codeigniter4:developfrom
mostafakhudair:patch-12

Conversation

@mostafakhudair

@mostafakhudairmostafakhudair commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

keeping configuration methods away of hands

@mostafakhudair
Removing getFormatter inside a new class
3f85503
@mostafakhudair
new Format Class
7fe8f96
creating new class Format to handle getFormatter method
@mostafakhudair
New Exception methods
44472e5
new exception methods forInvalidFormatter & forInvalidMime using in getFormatter
@mostafakhudair
added two translations
93b6d2b
adding invalidFormatter & invalidMime translation using in FormatException
@mostafakhudair
respect new Format Class
1a95f9f
adding some changes to current format codes to be compatible with \CodeIgniter\Format
@mostafakhudair
respect new Format class
affcecb
adding some changes to current format codes to be compatible with \CodeIgniter\Format
@mostafakhudair
adding new service
df4e1c4
adding format as instance of factory for Formatters
@mostafakhudair
using format() as service
fae6f90
michalsn
michalsn requested changes Jun 22, 2020
View reviewed changes

@michalsnmichalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • We need some tests for the new Format class.
  • ResponseTrait needs an update to use a new Format class.

Comment threadapp/Config/Format.php
*
* @return \CodeIgniter\Format\FormatterInterface
*/
public function getFormatter(string $mime)

@michalsnmichalsnJun 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can't remove this. It would be a BC change.

This should use Services::format() under the hood and have a phpdoc comment that points out this method is deprecated since we have a new Format class now.

@mostafakhudairmostafakhudairJun 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I didn't remove it completely i just relocated it in system/Format/Format.php under CodeIgniter\Format namespace and now all Format instances runs throw \Config\Services::format()

about tests I actually don't know how to do it on the right way, may i get help or something

@michalsnmichalsnJun 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I'm aware that you moved it to the Format class, but it doesn't change the fact that some people might use this method already in their code (just like we do in some of our tests). If we remove this method from here it will break their code - it has to stay.

@mostafakhudairmostafakhudairJun 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I got your point, but i think we can solve this problem by hinting new changes in next version changelog or something like that, any ideas?

@michalsnmichalsnJun 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, sorry. It has to stay - it was a public method. Removing it will be a breaking compatibility change.

Comment threadsystem/HTTP/Response.php Outdated
Comment threadsystem/HTTP/Response.php Outdated
Comment threadsystem/HTTP/Response.php Outdated
Comment threadsystem/HTTP/Response.php Outdated
Comment threadsystem/HTTP/Response.php Outdated
@mostafakhudair
Update Response.php
d10f28d
@mostafakhudair
respect new Format Class
15a75f6
adding some changes to current format codes to be compatible with \CodeIgniter\Format
removing negotiate() methord 3rd argument its false by default
removing useless property $this->formatter
@mostafakhudair

mostafakhudair commented Jun 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Travis faild i will do changes at night

michalsn
michalsn requested changes Jun 23, 2020
View reviewed changes
Comment threadsystem/API/ResponseTrait.php Outdated
Comment threadsystem/API/ResponseTrait.php
Comment threadsystem/API/ResponseTrait.php Outdated
Comment threadsystem/HTTP/Response.php Outdated
Comment threadsystem/API/ResponseTrait.php Outdated
@michalsn

michalsn commented Jun 23, 2020

Copy link
Copy Markdown
Member

If you look at Travis build, you will see some errors. Please change the code in these tests so they use new Format class instead of Config\Format::getFormatter()

Speaking about tests for Format class - I think taking a look at LoggerTest would be a good idea, because we have there quite simple, generic examples. You should test all simple things you can do with this new class, like loading formatters, throwing exceptions when it should be thrown etc.

Of course, test it locally first: ./vendor/bin/phpunit tests/system/Format/Format.php - this will be probably a call to your test for Format class. Take your time and browse other tests we already have - they will certainly explain a lot.

@mostafakhudair
adding getConfig method
0ac8ede
but shall I use (! empty($this->config) or just ! instanceof as I did
@mostafakhudair
improvements
2f35d65
revert some changes and improve code style
@mostafakhudair
Reducing lines
f513df0
One line code
@mostafakhudair
use format() service
dc9bf9a
@mostafakhudair
improvements and some fixes
bea491b
replace with new format() service
@mostafakhudair
Update FeatureResponseTest.php
20906ac
@paulbalandanpaulbalandan mentioned this pull request Aug 29, 2020
Merged
5 tasks
@mostafakhudair
mostafakhudair deleted the patch-12 branch September 4, 2020 14:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michalsnmichalsnmichalsn requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mostafakhudair@michalsn