Closed niden closed 4 years ago
All checked doc's are below.
ACL
ASSETS
CACHE Perfect :D
@ruudboon
ACL: Maybe add an example of the ACL firewall also.
I was going to add a page on its own for that but could very well add it to the same page. I guess we can leave this as is now, work on the new firewall
page and then decide if we want to cross link them or include the Firewall
in Acl
In the top of the page we sum the options, can we internal link this (For example to https://docs.phalconphp.com/4.0/en/assets#auto-versioning)
I have that in the API documentation. The reason I opted not to is because we have the left menu that does that and if we add the links at the top it pushes the content downwards which is not ideal.
Coding standard looks good to me. Asset can also be marked done
Completed Config
Completed Security
Reviewed collection. Not my favorite colors in the example but all clear and ready for production.
@ruudboon LOL. Well it matches 1776
....
Thanks for reviewing that.
Hi, #2345 sponsors update will be like this https://docs.phalconphp.com/3.4/en/sponsors
I reviewed config. Dropped a pull for some small changes. One thing that is a bit unclear to me is the start example. From the code examples I understand that getenv('APP_NAME') should hold "PHALCON". Maybe it's a better idea to just put strings there with the value.
Contributions looks good to me.
Completed Url
Completed i18n
Verified
Verified Helper All good only would like to check reduceSlashes. What are the rules for accepting double slashes? I see that http://url won't change but we need to explain this a bit more. Will ftp://url work? And how about an url like //your.domain.com
Verified logger Can we explain what will happen if one of the loggers fails when using multiple loggers. Would be cool if it could still write to the other loggers. But that's outside the scope. I do think we should describe what will happen.
new-feature-request All good
new-pull-request Can we add something that this is about the Phalcon source and we have different requirements for the docs?
registry All good
reproducible-tests All good
security See pull And end of document. Also in your views (Volt syntax). I think this is only the case with MVC or is this also available on micro if you setup volt?
testing-environment All good
translate All good
And end of document. Also in your views (Volt syntax). I think this is only the case with MVC or is this also available on micro if you setup volt?
Yes so long as you set volt up it is there.
All good only would like to check reduceSlashes. What are the rules for accepting double slashes? I see that http://url won't change but we need to explain this a bit more. Will ftp://url work? And how about an url like //your.domain.com
The schema slashes are left as they are:
echo Phalcon\Helper\Str::reduceSlashes("http://foo.bar///baz/buz");
http://foo.bar/baz/buz
The //something/somethingelse
will not work since it is not a real URL.
ftp://something
works
The regex is: preg_replace("#(?<!:)//+#", "/", text);
All good only would like to check reduceSlashes. ....
Added more examples
Can we explain what will happen if one of the loggers fails when using multiple loggers. Would be cool if it could still write to the other loggers. But that's outside the scope. I do think we should describe what will happen.
Added a small callout for this.
i18n Reviewed i18n, nothing incorrect but pure PHP examples. Wondering if we can make examples that really involves some Phalcon. Maybe in combination with volt?
url Just one typo (see pull) rest is perfect
Reviewed i18n, nothing incorrect but pure PHP examples. Wondering if we can make examples that really involves some Phalcon. Maybe in combination with volt?
Well there is not much to add there since this is all PHP functionality, Phalcon does not help anywhere there. Honestly this article has been written years ago and I could never see the real reason behind it since it is just a discussion about the intl
extension
@niden I already scanned some docs. Hopefully it can be at help. We need to verify all the code examples to match v4 but besides that some points.
Annotations In general it looks good to me.
Application-cli Some small things we can look at.
Application In general ok.
Application-micro Looks good to me
Controllers Looks good
Cookies Looks good
DB-Layer Looks good
Loader
completed
Loader reviewed! All good. thnx
Since Phalcon\Plugin
got its own namespace, it could be useful to document it as well.
Thnx @diplopito Added it to the list
As discussed on Discord @diplopito will have a look at:
Thnx!
Reviewed request.md in general it looks great. Got some small questions.
In the beginnen we mention there are 4 (should be 5, see pull) for retrieving data. But you can also use getFilteredPost(), getFilteredPut(), getFilteredQuery(). Should we mention this there also? How about the $_FILES maybe introduce something here also?
getClientAddress(): Gets most possible client IPv4 Address. Only v4? What will happen if it's a v6 request and how to get it?
Uploading Files Is this the correct name? We're talking about the request. Was expecting something like Handeling file uploads. For 4.1 it would be nice if we could extend this file uploading a bit and give some filtering options here also. Like extension/mime check etc.
Response done cc @ruudboon
In the beginnen we mention there are 4 (should be 5, see pull) for retrieving data. But you can also use getFilteredPost(), getFilteredPut(), getFilteredQuery(). Should we mention this there also? How about the $_FILES maybe introduce something here also?
We mention those a bit further down in sanitized presets
getClientAddress(): Gets most possible client IPv4 Address. Only v4? What will happen if it's a v6 request and how to get it?
This is whatever comes from the $_SERVER
array. If that one has ipv6 then it should be sent back. Maybe we need to test this a bit.
Uploading Files Is this the correct name? We're talking about the request. Was expecting something like Handeling file uploads. For 4.1 it would be nice if we could extend this file uploading a bit and give some filtering options here also. Like extension/mime check etc.
Changed the name to uploaded
files. As for the rest we can create a NFR for 4.1
Reviewed Response.md Looks good. And pushed a small change. One question: https://docs.phalcon.io/4.0/en/response#overview
Note that this is not only the actual response payload. I'm not exactly sure what you mean with this. Other parts of code can return also? Like doing echo and flush?
Note that this is not only the actual response payload. I'm not exactly sure what you mean with this. Other parts of code can return also? Like doing echo and flush?
The actual response is a specific payload that you can use PSR-7 with (http-response.md
) That is what you create and then you use a client (say Guzzle) to send it back. The Response
object sends the data back also so it acts similar to a response generator and client to send the response.
The actual response is a specific payload that you can use PSR-7 with (http-response.md) That is what you create and then you use a client (say Guzzle) to send it back. The Response object sends the data back also so it acts similar to a response generator and client to send the response.
Ah ok. Can we add something like this to the intro?
Updated
PSR section in the docs
cc @ruudboon
Checked debug. Looks good. An update of the screencast would be a nice to have. Flash is all ok
escaper Can we add a list of supported encoding (what can we expect from getEncoding())
http-request https://docs.phalcon.io/4.0/en/http-request#constructor Are the optional parameters passed in as an array?
@ruudboon getEncoding
will just return what you used in setEncoding
. it is a simple getter/setter. If the encoding is not a valid one for the mb_*
functions then an exception is thrown.
For the request and other documents the format for methods I used is similar to the one that PHP does. They put [ ]
between optional parameters
@niden Ok. Was expecting something like this.
public function __construct(
[string $method = "GET"] ,
[mixed $uri = null],
[mixed $body = "php://temp"],
[array $headers = []]
)
But make sense that you need to specify that if you want to set $uri you need to set $method also
This is where I got it from:
https://www.php.net/manual/en/function.mb-check-encoding.php
Done and ok
http-response In the example we load a txt file but output a json without doing any json encoding of the text file. It shows how it works but I think it will be nice to sent for example a txt header. Maybe to prevent discussion can we add something more general here instead of the amendment? (contributors.txt ?)
http-server-request http-stream http-uploaded-file Change bill-of-rights.txt ?
https://docs.phalcon.io/4.0/en/http-stream#write Can we change this example?
performance
Can we make performance
red to indicate it's not a good idea?
I think we can remove CDN from the bullet points. CDN is more a solution to the above mentioned problems.
https://docs.phalcon.io/4.0/en/performance#server Should the title be profiling?
Can we add a disclaimer? The suggestion we give are good practices it will depend on the type of application if this should be applied or not.
use-case Looks good, but not sure if all our code examples align.
https://docs.phalcon.io/4.0/en/db-models-behaviors All good, one small thing. Can we add the note in the bottom something about using traits won't allow you to use beforeUpdate methods etc in your model. I think this is the main reason why not to use traits.
https://docs.phalcon.io/4.0/en/db-models-cache Can we make it a bit more clear that we are talking about database caching? I don't think we serialise and cache the model.
Can we rephrase this a bit?
This is due to the complex connection and communication processes that PHP uses with each request to obtain the data from the database. The issue intensifies if the data queried is complex and involves multiple joins on tables.
- Don't think the communication complexity is related to PHP. This is for every language.
- "The issue intensifies if the data queried " This is a different issue and not related to the connection mentioned before.
If we want to increase performance, we will need to implement some layers of caching where required
- Don't think caching is the only solution. Maybe we rephrase this to something like. "Implement some layers of caching reduces the number of connections and lookups that help increasing performance"
PHQL Queries, wasn't expecting this to find this here in the model cache
In the example we load a txt file but output a json without doing any json encoding of the text file. It shows how it works but I think it will be nice to sent for example a txt header. Maybe to prevent discussion can we add something more general here instead of the amendment? (contributors.txt ?)
Fixed
http-uploaded-file Change bill-of-rights.txt ?
Fixed
https://docs.phalcon.io/4.0/en/http-stream#write: Can we change this example?
Fixed
performance
Can we make performance red to indicate it's not a good idea?
Where?
I think we can remove CDN from the bullet points. CDN is more a solution to the above mentioned problems.
Fixed
https://docs.phalcon.io/4.0/en/performance#server Should the title be profiling?
These are server related activities hence the heading.
Can we add a disclaimer? The suggestion we give are good practices it will depend on the type of application if this should be applied or not.
Fixed
https://docs.phalcon.io/4.0/en/db-models-behaviors: All good, one small thing. Can we add the note in the bottom something about using traits won't allow you to use beforeUpdate methods etc in your model. I think this is the main reason why not to use traits.
What do you mean by that? If I add the beforeUpdate
method in a trait it will be able to run in a model. Maybe I am not getting it.
https://docs.phalcon.io/4.0/en/db-models-cache: Can we make it a bit more clear that we are talking about database caching? I don't think we serialise and cache the model.
Not sure I understand the above - where?
Can we rephrase this a bit?
Check it out please
PHQL Queries, wasn't expecting this to find this here in the model cache
This is because PHQL queries return models. I have a section in db-phql also
https://docs.phalcon.io/4.0/en/tutorial-invo Looking at the code I don't think we serve at /invo/ but on /. Would be good to serve from a directory but probably we need to change the input of the handler. Rest of tutorial looks great
This has been addressed
WIP
Docs
Completed/Reviewed
Notes We need to make sure that all issues in the cphalcon repository with the label "docs needed" are covered in our documentation.