-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Add a method to TemplateProcessor for rendering HTML content.Include Image #2547
base: master
Are you sure you want to change the base?
Conversation
@Maybe-U You asked me to review. The code looks okay to me, but I'm really not all that familiar with TemplateProcessor, so it needs someone more knowledgeable to review. However, your test may have a problem. Does it actually attempt to read your external images? If the answer is no, there's no problem. If the answer is yes, I'll add some more comments. |
this is output docx file @Progi1984 can you review my code thanks you |
My testing indicates that your test does attempt to load image https://t7.baidu.com/it/u=4198287529,2774471735&fm=193&f=GIF and will fail if it is not available; that is probably true of your second image as well. The problem is that external files tend to move or be deleted over time, and, when (not if) that happens, the unit test suite will fail. To avoid this problem, I think you might be able to use |
|
if (!empty($imageSrcList)) { | ||
foreach ($imageSrcList as $imageSrc) { | ||
try { | ||
$content = file_get_contents($imageSrc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section of the code is quite clever, but (a) I'm not really sure it's needed, and (b) I don't think you've coded it correctly. I would personally eliminate all the code after addSection
and before addHtml
and just let it fail later if the file isn't available; your changes to your test member are sufficient to address my original concern by making the files local rather than external. But, I can see that this checking might be perceived as a benefit, so let's discuss (b). file_get_contents
does not normally throw an exception (I am not expert in the Php internals, so I suppose there might be some edge case where it throws); it normally just returns false
if it fails and issues some warning messages describing the failure. So, I think what you want to do is:
foreach ... {
$content = @file_get_contents(...); // suppress warning messages
if ($content === false) {
$localImg = ...
$htmlContent = ...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks you for your Suggestion,I wasn't thoughtful enough ,I reviewed the html::add html method,Use @ to suppress errors when processing images in this method
so i eliminate all the code after addSection and before addHtml Perhaps the abnormal picture should be handled by the user itself.
In addition, this is my first time to submit pr. Could you please help me merge this pr
@oleibman
<w:sectPr><w:pgSz w:orient="portrait" w:w="11905.511811023622" w:h="16837.79527559055"/><w:pgMar w:top="1440" w:right="1440" w:bottom="1440" w:left="1440" w:header="720" w:footer="720" w:gutter="0"/><w:cols w:num="1" w:space="720"/></w:sectPr> Due to the section's properties are stored in the sectPr element above. The document style changed. $documentBodyStr = preg_replace('/<w:sectPr>.*?<\/w:sectPr>/', '', $documentBodyStr); |
Description
Add a method to TemplateProcessor for rendering HTML content. support image
Fixes 1