Move General::buildPaginationElement >> DataSource #2685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendo
reviewed
May 30, 2017
| @@ -581,7 +581,9 @@ public function execute(array &$param_pool = null) | |||
| $result->prependChild($sectioninfo); | |||
|
|
|||
| if ($include_pagination_element) { | |||
| $pagination_element = General::buildPaginationElement(); | |||
| $pagination_element = $this->buildPaginationElement(array( | |||
There was a problem hiding this comment.
Use Datasource:: to access the method (because it's static).
brendo
reviewed
May 30, 2017
| @@ -1494,6 +1494,7 @@ public static function createXMLDateObject($timestamp, $element = 'date', $date_ | |||
| /** | |||
| * Construct an XML fragment that describes a pagination structure. | |||
| * | |||
| * @deprecated @since Symohony 2.7.0. Use DataSource::buildPaginationElement() | |||
There was a problem hiding this comment.
Two typos, Symohony (Symphony) and DataSource (Datasource)
brendo
reviewed
May 30, 2017
| * @return XMLElement | ||
| * the constructed XML fragment. | ||
| */ | ||
| public static function buildPaginationElement(array $infos = array()) |
There was a problem hiding this comment.
I feel the old signature was cleaner, what prompted the change?
There was a problem hiding this comment.
In order to make function signature resilient to changes, its a good practice to try to replace parameter with defaults values with a "settings bags".
You can then
- Specify the 3rd option only, and keep the default intact.
- Add a new option without worrying about signatures/API compat
- Extension could ship there could with new option and it should not break on old install!
It only needs to be documented
Those parameters was the perfect example of this code smell and I did wanted to try it out.
There was a problem hiding this comment.
Most of the Widget would benefit from this... $value, $attributes.
There was a problem hiding this comment.
That is a really interesting perspective, thanks for sharing!
There was a problem hiding this comment.
The same pattern exists in javascript too, where you have an "configuration object" that merges with defaults ;) Like most jQuery plugins
brendo
reviewed
May 30, 2017
| @@ -241,6 +241,45 @@ public function emptyXMLSet(XMLElement $xml = null) | |||
| } | |||
|
|
|||
| /** | |||
| * Construct an XML fragment that describes a pagination structure. | |||
| * Do not call as a static function, since this is only temporary. | |||
There was a problem hiding this comment.
It SHOULD be static, but it can't right now (it was crashing on my end...) Maybe I need to revisit this.
michael-e
reviewed
May 30, 2017
| @@ -1508,16 +1509,14 @@ public static function createXMLDateObject($timestamp, $element = 'date', $date_ | |||
| * @return XMLElement | |||
| * the constructed XML fragment. | |||
| */ | |||
| public static function buildPaginationElement($total_entries = 0, $total_pages = 0, $entries_per_page = 1, $current_page = 1) | |||
| public static function buildPaginationElement($total_entries = 0, $total_pages = 0, $entries_per_page = null, $current_page = 1) | |||
There was a problem hiding this comment.
Why null? Shouldn't it be 0 (because we assign integer values)?
michael-e
previously approved these changes
May 30, 2017
nitriques
force-pushed
the
2.7.x
branch
2 times, most recently
from
c443ded
to
8bce579
Compare
This is needed to be able to run the build without having anything installed globally. The README got updated but we need to update the wiki: https://github.com/symphonycms/symphony-2/wiki/Contributing-to-Symphony https://github.com/symphonycms/symphony-2/wiki/Building-Symphony's-Assets https://github.com/symphonycms/symphony-2/wiki/Building-Symphony Re symphonycms#2717 Picked from 499b82b Picked from 893e29e
The second cond is not mandatory, since the first one will catch everything. Fixes symphonycms#2730 Picked from 194a546 Picked from f308df9
We already provided the 'simple' SQL clause to check for nullable, but it was not possible to combine null checks with another filter, like so: earlier than today, is null credits to @PascalPiche Picked from 16a58f4 Picked from e55d6db
Keeping only the one with the section handle + field handle.
from :
<entry id="6" greetings-test="0" greetings="0" />
<entry id="5" greetings-test="0" greetings="0" />
<entry id="4" greetings-test="0" greetings="0" />
to :
<entry id="6" greetings-test="0" />
<entry id="5" greetings-test="0" />
<entry id="4" greetings-test="0" />
Picked from 7e4a829
Re symphonycms#2610
Picked from b05cf17
This is to prevent over rotating the logs Fixes symphonycms#2757 Picked from 8a9a578 Picked from d69537b
Because the order of import is important Picked from e86ca21
Picked from b12dc10
Picked from 16dbcc2
Picked from c7a13b0
Picked from 64402fd
This file allows security researchers to find us quickly if they find a security related problem. See https://securitytxt.org Fixes symphonycms#2807 Picked from 7cad4b7
This commits add a new parameter in data sources to control if the output parameters should be added in the xml output. By default, data sources that do not have the dsParamPARAMXML property set will include their output parameters in the XML, since this is their current behavior. If the dsParamPARAMXML exists on the object, it only needs to be different from 'yes' to be excluded from the XML. Fixes symphonycms#784 Picked from d9ea144
…ycms#2597) * Fix doc: E_NOTICE are loggued c8ff3dc The doc stated that E_NOTICE were ignored while the code only ignores E_STRICT (see line 423) * Make renderHtml protected ebc5ac7 This method should never get called directly. Nothing ever calls it in the current base code. * Disable GenericExceptionHandler before auth d53b221 Those changes makes sure we do not leak exception information before doing the authentication. It also makes sure that exception raised in the exception handler won't leak info if the GenericExceptionHandler is disabled. The same logic is also applied in the shutdown function. Instead of masking the error like we did, the logic is applied only in the render part of things, which should make it easier for us to debug. It also removes the wrong HTTP status that we were returning (500 instead of 404) Picked from dbcf209 * Add GenericExceptionHandler::$logDisabled dec526d This implementation copies what was already present in the GenericErrorHandler, i.e. skip logging if disabled = true. This also prevents errors caught in GenericErrorHandler to be also loggued a second time in the GenericExceptionHandler::handler method. * Check GenericExceptionHandler::$enable in subclass 20f37aa Both SymphonyErrorPageHandler and DatabaseExceptionHandler needed care: SymphonyErrorPageHandler: - throwCustomError must not touch GenericExceptionHandler::$enabled since it allows data leakage - defaults the diabled and not template cases to GenericExceptionHandler::render DatabaseExceptionHandler: - Filter out data if the Handler is not enabled. This handler was already getting the right template since it uses the same as the parent. GenericExceptionHandler: - refactored the headers part to share code in SymphonyErrorPageHandler * Made handlers final 8d84a7d Both the GenericErrorHandler and the GenericExceptionHandler's handler function should not be overriden. * FrontendPageNotFoundExceptionHandler bypass 809f927 FrontendPageNotFoundExceptionHandler can bypass the new anti-leak system since it's a legitimate message. Picked from 3da6eeb
As discussed in symphonycms#784, it may be useful to output system IDs only. Picked from f843d96 Picked from 5ccc9f0
This allows developers to act on when those values changes Picked from 775fbae
This allows developers to act only when those values changes¸ and that the author was properly saved. Re 775fbae symphonycms@775fbae#r28055757 Picked from 1954393
* Add the Content-ID header on attachments d7abb0e This allows developer to reference attachments in the html body to allow images to be viewed offline. You can use it as follow: ```html <img src="cid:name-of-the-file.ext" /> ``` Where the name-of-the-file.ext part matches the actual file name of the file. Notice the `cid:` prefix, which makes it work! Picked from 2b75aa7 * Add a flag to enable Content-ID header field 5483f71 * Let the developer controls the disposition value c3a6a27 * Let the developer controls the content-id value 8710309 * Remove null keys from array and add isset() 24786e1 Also remove the old undocumented format * Add missing commas 5c75992 Picked from 895d7cb
Both events and data sources were missing the PostDelete delegates. No delegates were fired when the resources were deleted from the index page. The handle of the resource is added to PreDelete also. Re symphonists/globalresourceloader#13 Picked from 8104058 Picked from 2622f49
* Remove parameters as arguments d3c31c3 i.e. using $param in XSLT without declaring it. Fixes symphonycms#2338 * Redistribute responsibilities between Page/Process 0614ff5 We used to store the xml and xsl values both in the XSLTPage AND in the XSLTProcess. This is redundant and totally not needed. This change makes only the Page responsible to hold the xml and xslt data, which are simply passed when the XSLTProcess needs to do it job. The only reason why the Process would need to hold the data is when there is an error. For this purpose, the "context" is managed manually instead of exposing it publicly. On the other hand, the Page was holding data belonging to the Process: params and functions. Since the page does not use the parameters anymore and it does not register any functions, this data has been moved to the process. Upon instanciation, the Page gets a process, so developers can just forward their Page calss to Page->Proc. The end goal was to not expose any XSLT parameters or function in the XSLTPage, which is now achieved. Picked from f715cf2
General is a already quite loaded. This method those not belongs there. This commit replaces the implementation in General with a call to DataSource. The General method is marked as deprecated. The General method now calls the new implementation directly. The new implementation uses an array to receive optional parameters via an array, which makes the implementation a bit better. Fixes symphonycms#2670
nitriques
force-pushed
the
3.0.x
branch
2 times, most recently
from
7a1993b
to
6f23cc0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit details.
Fixes #2670
(Do not merge for now)