-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
Change type of access for methods #24
Comments
Which concrete method do you need and for which case? Blindly changing all methods to protected is a bad idea. |
Это не плохая идея т.к. все методы\свойства взаимосвязаны. Я хочу хранить данные в кеше, а не в файлах, следовательно мне нужно переопределить минимум Index: write() + Sitemap: createNewFile(), flush(), но само собой эти методы работают с другими методами и переменными которые что? - правильно! закрыты... |
As a variant: you can add a new branch and implement it in version that will return content (not saved in files). |
Опишите свой кейс. Для чего именно вам хранить sitemap в кеше? Как вы его отдаёте? |
Кеш (https://github.com/kohana/cache). |
У вас XML в кеше? |
У меня xml текст в файловом кеше и каких-либо проблем по этому поводу я не испытываю. Основная мысль - дать возможность пользователю самому решать как ему хранить данные, не вижу смысла в подобном ограничении расширяемости, можно провести небольшой рефакторинг и отделить момент генерации от сохранения контента. |
У Вас висит пару месяцев PR #16, пользователь не может реализовать сжатие в своем варианте, поэтому вынужден расширять Ваш, а Вы как я понимаю этого не хотите. Как итог: смысл библиотеки теряется. |
Генерацию от сохранения отделить можно, да. Но всё делать protected я против. |
Ты сам посмотри что в итоге останется |
Я уже начал над этим работать, но с удовольствием посмотрел бы на альтернативное решение. |
Сомневаюсь что оно будет альтернативным, но ради интереса попробую. |
I want to extend the classes and override the write() and flush() method so that I can have it write to Amazon S3 instead of the local filesystem. I can't do that because the methods are marked private instead of protected. I think getCurrentFilePath() and $this->writer would also need to be protected. |
@ejunker I think it's better to abstract writing into separate interface / class. If you have ideas about it, let's discuss it. |
@samdark @WinterSilence Any news on this? We have a use case where we just want the xml to be rendered using
That is the general idea. I have seen other issue where complete new solution is suggested. Any news on that if any implementation is being done or in process. |
No implementation was done or is in progress. While it's OK to have an ability to write to any stream, it's not clear what's your particular use case. |
@samdark I don't want to use |
That's a very bad idea:
|
|
|
|
|
|
I think it's a bad idea to submit just new URLs and omit old ones: https://webmasters.stackexchange.com/questions/2459/should-i-include-everything-in-the-sitemap-or-only-new-content. No search engine guarantees that if your page reached index it will stay there forever. |
Got it. But I think its a separate issue while thinking of |
Well, it was designed to work with files. For example, it advanced to next file when current one is full: https://github.com/samdark/sitemap/blob/master/Sitemap.php#L227. I'm not sure how to do that if you've specified a stream that's no a file. |
Nothing happens as said earlier, writer just flushes the stream and carries on with the processing
|
Carries on with processing where? For files it opens a new file. Continuing flushing to output in this case would result in incorrect sitemap that won't be parsed by search engines. It doesn't make sense to me. Same about memory. You'll need a sitemap index and a set of sitemaps but these are read in multiple requests so you can't direct these into streams. |
It will output all the stream to |
But that basically won't be read when you reach 50000 elements or a 10 MB of data and you have to write all URLs into sitemap. |
Agreed, but we don't have a use case of |
If you have FS, why do you even need to generate anything runtime at all? |
Right now, we don't have any need to use |
I see. Well, sorry but I don't think having a way to easily shoot into your own leg is a good idea. Therefore, do your own fork. |
Change type from
private
toprotected
for extending classes.The text was updated successfully, but these errors were encountered: